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

[Abel Bernabeu via Phabricator via llvm-commits]

abel-bernabeu added a comment.

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 fix just the occurrences that were deemd needed for most common cases.

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.

Second objection is that there are reviewers missing that gave useful feedback for the older merge request.

5. Sergei Barannikov will not be mentioned on the history, when we are just applying, testing and committing his generic way 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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D153204