[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 533194. abel-bernabeu added a comment. Even less verbosity... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 Files: llvm/lib/Target/RISCV/AsmParser/RISCVAsmP

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 533193. abel-bernabeu added a comment. Reduced the commit message verbosity by one half, without losing anything. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 533191. abel-bernabeu added a comment. Addressed some review comments: - Doxygen-style comments are not common, so using triple-slash instead. - Using a regular C array on a place where the usage of SmallVector is not justified. - Proper capitalization

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

barannikov88 added inline comments. Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1610 +} + } while (NonComments < 2 and ReadCount > 0); + return NextNextToken; This is much more common. Repository: rG LLVM Github Monorepo CHANGES SINC

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

craig.topper added inline comments. Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:1586 +/** +\brief Peeks next after next token I don't think this style of comment is common in llvm. Comment at: llvm/lib/Target/RISCV/AsmParse

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532837. abel-bernabeu added a comment. I just this flashback... For code coverage of one of the Lex() calls in ParseInstruction we needed one case where an instruction has no operands. We had, not one, but three. Am keeping just the "nop" case, which

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. @jrtc27: The tests: - are under llvm/test/MC/RISCV - use llvm-mc as assembler (rather than C!) - are as simple as they can be - cover every line touched by this patch. Would you unblock this now? Thanks for your feedback so far. @MaskRay: Let me know if you miss

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532758. abel-bernabeu added a comment. On the zdinx test, changed a FileCheck comment that was written starting with "//" rather than "#". Changed for "#" just for consistency with the rest of tests. Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu accepted this revision. abel-bernabeu added a comment. Am happy with the current patch version Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 ___

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532754. abel-bernabeu added a comment. Removed a gratuitous blank line Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 Files: llvm/lib/Target/RISCV/AsmParser/

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532753. abel-bernabeu added a comment. Added --match-fulllines to FileCheck for making the check stricter. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 Files

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added inline comments. Comment at: llvm/test/MC/RISCV/comments.ll:25 +# CHECK: .Lpcrel_hi0:#c0 #c1 #c2 #c3 #c4 #c5 #c6 +# CHECK: auipc s0, %pcrel_hi(symbol+10) +# CHECK: addis0, s0, %pcrel_lo(.Lpcrel_hi0) This shoul

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532686. abel-bernabeu added a comment. Rebased on top of the latest changes in RISCVAsmParser.cpp Moved the testing to llc-mc test cases under lvm/test/MC/RISCV/ Covered with tests for every single case where the parser consumes a token and a potential

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

barannikov88 added a comment. - I'd prefer Lex() over equivalent getParser().Lex() because it is shorter. - Ideally, every change should be tested. - The tests should be minimal. That is, they should be assembly files passed to llvm-mc rather than ll files passed to llc. I'm not very familiar wi

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532489. abel-bernabeu added a comment. Fixed a typo on commit message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 Files: llvm/lib/Target/RISCV/AsmParser/R

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532488. abel-bernabeu added a comment. Updated commit message: - fixed a typo - added a co-author Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 Files: llvm

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. In D153008#4430491 , @MaskRay wrote: > In D153008#4428694 , @abel-bernabeu > wrote: > >> In D153008#4428568 , @MaskRay >> wrote: >> >>> Th

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

MaskRay added a comment. In D153008#4428694 , @abel-bernabeu wrote: > In D153008#4428568 , @MaskRay wrote: > >> This new update still applies many unneeded `getParser().Lex();` and adds a >> test at a wrong laye

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. In D153008#4424821 , @jrtc27 wrote: > Clang tests should not compile to asm. You want an IR test. My bad. You suggested IR as input (rather than C) and makes total sense. The latest update captures that suggestion. Repos

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532268. abel-bernabeu added a comment. The test has been moved to llvm/test/CodeGen/RISCV Also has been reworked for using LLVM IR as input, so clang is not needed in the loop. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. In D153008#4428568 , @MaskRay wrote: > This new update still applies many unneeded `getParser().Lex();` and adds a > test at a wrong layer >

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu marked an inline comment as done. abel-bernabeu added a comment. In D153008#4428568 , @MaskRay wrote: > This new update still applies many unneeded `getParser().Lex();` and adds a > test at a wrong layer >

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. In D153008#4425244 , @jrtc27 wrote: > In D153008#4425238 , @abel-bernabeu > wrote: > >> In D153008#4424821 , @jrtc27 wrote: >> >>> Clang te

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

MaskRay added a comment. This new update still applies many unneeded `getParser().Lex();` and adds a test at a wrong layer (clang/test/CodeGen): Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 532185. abel-bernabeu added a comment. Simplified the test. Also I check that the comments are a carried over to the assembly output. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://rev

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

MaskRay added a comment. You may add a test file beside `llvm/test/CodeGen/RISCV/inline-asm*`. I hope that this patch focuses on `getLexer().Lex()` instances that actually cause a problem. Many `getLexer().Lex()` instances followed by `is(...)` form a pattern that can be rewritten in a better

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. Thanks everyone, taking notes of all the comments for improving the test: - Simplify the test (can be one instruction, no problem). - Check at IR level - Check for the comments to be placed in the output - Do "arc diff" with one-line descriptions Will update before

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

jrtc27 added a comment. In D153008#4425238 , @abel-bernabeu wrote: > In D153008#4424821 , @jrtc27 wrote: > >> Clang tests should not compile to asm. You want an IR test. > > Jessica, are there any exceptions for

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. In D153008#4424821 , @jrtc27 wrote: > Clang tests should not compile to asm. You want an IR test. Jessica, are there any exceptions for tests is under CodeGen/RISCV intended to exercise the assembly parser? I have just wr

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

jrtc27 requested changes to this revision. jrtc27 added a comment. This revision now requires changes to proceed. Clang tests should not compile to asm. You want an IR test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

barannikov88 added a comment. Herald added a subscriber: wangpc. Does the test have to be a C source? Why not just plain asm (fed into llvm-mc)? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153008/new/ https://reviews.llvm.org/D153008 ___

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

DavidSpickett added inline comments. Comment at: clang/test/CodeGen/RISCV/riscv-inline-asm-gcc-commenting.c:23 +// CHECK: f1: +// CHECK: lui a0, 1 +// CHECK-NEXT: addiw a0, a0, 564 Check for the comment content here too? `# this is fine # this

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 531746. abel-bernabeu added a comment. [RISCV] Allow slash-star comments in instruction operands It has been reported by one of Esperanto's customers that slash-start comments ("/*") within inline assembly were only allowed before the first instruction

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 531741. abel-bernabeu added a comment. [RISCV] Allow slash-star comments in instruction operands It has been reported by one of Esperanto's customers that slash-start comments ("/*") within inline assembly were only allowed before the first in

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. I was a bit reluctant to start a discussion on migrating getLexer().Lex() to getParser().Lex(), but I guess it makes sense to do it now rather than deferring. It is cleaner now, I agree. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 531740. abel-bernabeu added a comment. [RISCV] Allow slash-star comments in instruction operands It has been reported by one of Esperanto's customers that slash-start comments ("/*") within inline assembly were only allowed before the first instruction

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu updated this revision to Diff 531735. abel-bernabeu added a comment. [RISCV] Allow slash-star comments in instruction operands It has been reported by one of Esperanto's customers that slash-start comments ("/*") within inline assembly were only allowed before the first in

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

rogfer01 added a comment. Hi @abel-bernabeu, I did a quick experiment replacing all the calls to `getLexer().Lex()` with `getParser().Lex()` and now your testcase is accepted and looks like this in the output f2: addisp, sp, -32 sd ra, 24(sp) sd s0, 16(sp

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu added a comment. For the customer who reported the problem, the comments are in the input source (doing their job explaining what the operands are). Now, if that comment is not seen when compiling with "-S" it is less of a problem that having the compilation not succeeding. I did

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

DavidSpickett added a reviewer: DavidSpickett. DavidSpickett added a comment. Is the comment here relevant? https://discourse.llvm.org/t/interleaving-several-c-style-comments-in-the-same-inline-assembly-line/71353/8 Does this patch do that already? Also is it a problem that the ignored comments

[PATCH] D153008: [RISCV] Allow slash-star comments in instruction operands

abel-bernabeu created this revision. Herald added subscribers: jobnoorman, luke, VincentWu, vkmr, frasercrmck, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jones, zzheng, jrtc27, shiva0217, kito-cheng, niosH