[PATCH] D105720: [AsmParser] Add support to LOCAL directive.

Tue Jul 13 16:12:50 PDT 2021

jcai19 added a comment.

In D105720#2872322 <https://reviews.llvm.org/D105720#2872322>, @MaskRay wrote:

> DenseSet<CachedHashStringRef> may be better than StringSet. The former doesn't need to duplicate the string.

Thanks!

> Does `LOCAL` (https://bugs.llvm.org//show_bug.cgi?id=45051) have a use case?
> As you mentioned, this can usually be replaced with local labels (`1:`)

Yes, the reported case should be fixed by the proposed workaround but sometimes it may be more difficult to find a workaround such as in the the test cases. While this might not be common, PR#45051 shows there still could be real-work use cases of this directive, and I think we should try to support it especially when it does not introduce significant change to the existing code.

> ---
>
> If the feature turns out to be useful,
>
> The implementation looks hackish to me. A proper implemention should make the `LOCAL` symbol similar to a macro argument.

That would look cleaner but I understand your commenttcorrectly, it is not easy to integrate the code into the following loop that handles macro arguments. For macro arguments, IAS could easily identify their uses by checking `\`. This does not apply to uses of `LOCAL` arguments, and if we would integrate the code, we need to check against possible LOCAL arguments at every character of `Body`, which would be more expensive.  The currently implementation goes through each use case of `LOCAL` arguments once (after it identifies the corresponding LOCAL directive call), while integrating it into the for loop would introduce (lenght of `Body` * possible LOCAL arguments) checks. WDYT?

> I think the replacement should use a `.LL` symbol instead. For non-RISC-V the local symbols are not intended to be retained in the symbol table.

Agreed, will update my code.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D105720/new/

https://reviews.llvm.org/D105720