[PATCH] D153204: RISCVAsmParser: support comments in more places

Tue Jun 20 21:57:52 PDT 2023

MaskRay added a comment.

In D153204#4431251 <https://reviews.llvm.org/D153204#4431251>, @abel-bernabeu wrote:

> In D153204#4430679 <https://reviews.llvm.org/D153204#4430679>, @craig.topper wrote:
>
>> In D153204#4430646 <https://reviews.llvm.org/D153204#4430646>, @abel-bernabeu wrote:
>>
>>> 1. Use "LI" rather than "VSETIVLI". It really does not matter what instruction you use for testing the handling of comments. Think of those backporting the patch to very old versions of LLVM.
>>
>> One of the changes is in the vsetivli specific parsing code.
>
> The change proposed by Sergei Barannikov was to just search and replace getLexer().Lex() for getParser.Lex() or simply Lex(), which is exactly what I did on the original merge request. This solution was preferred over surgically fixing just the occurrences that were deemed needed for most common cases.

I think this was a suggestion to help identify the problem, but not that the proposed patch should do this.

> 4. This patch, replaces just 5 out of 23 occurrences which are needed for the vsetivli instruction to get interleaved comments working. I do not see how this merge request is a better alternative to https://reviews.llvm.org/D153008 or why having the functionality only working for vsetivli is in any way better than just applying a clean search and replace for fixing every instruction.

I landed a couple commits to fix patterns like `if (getLexer().is(...)) { getLexer().Lex();` to use `if (parseOptionalToken(AsmToken::Minus))`. So the number of occurrences has decreased.

> 6. There are reviewers missing that gave useful feedback for the older merge request.

I took the list that usually reviews the changes. I left a comment on the other Differential, so those who want to review can come here with that notification.

> 5. Sergei Barannikov will not be mentioned on the git history, when we are just applying, testing and committing his generic solution for fixing the problem of comments interleaving on virtually any backend.
>
> Ultimately the code owners can fix in whatever way they feel is better, and as a newcomer I would stick to whatever is agreed here, but my personal vote is to reject this merge request and unblock the original one.

I am happy to add a thanks-to line, but this is probably not a situation that we use Co-authored-by:.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153204