[PATCH] D108602: [RISCV] Initial support .insn directive for the assembler.

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2270
+     RISCV::InsnI_Mem,
+     {MCK_UImm7, MCK_UImm3, MCK_AnyReg, MCK_SImm12, MCK__40_, MCK_AnyReg,
+      MCK__41_}},
----------------
jrtc27 wrote:
> I guess MCK__40_ is because of `'('` in ASCII... not immediately obvious
Yep. I'll add a comment.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2326
+
+  // Attempt to parse token as a register.
+  if (parseRegister(Operands, true) == MatchOperand_Success)
----------------
jrtc27 wrote:
> Does this not mean you can't, say, have `.insn b` with a symbol whose name happens to be a register? This seems like it needs to pay more attention to Kind.
Is that an existing bug in instruction parsing?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2344
+/// parseDirectiveInsn
+/// ::= .insn [ format, encoding, (operands (, operands)*) ]
+bool RISCVAsmParser::parseDirectiveInsn(SMLoc L) {
----------------
jrtc27 wrote:
> Unfortunately it *doesn't* have that first comma, it's a bit ugly like that, though I can see why that was chosen
Yeah, that was copied from SystemZ where I started this patch from before it snowballed. Thanks.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2428
+      // valid for a different match.
+      if ((it == EntryRange.first || ErrorInfo <= ActualIdx))
+        ErrorInfo = ActualIdx;
----------------
jrtc27 wrote:
> No assignment here, no double parens needed
Oops. That must have evolved out of hacking on the code emitted in RISCVGenAsmMatcher.inc


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1364
+
+let isCodeGenOnly = 1, hasSideEffects = 1, mayLoad = 1, mayStore = 1,
+    hasNoSchedulingInfo = 1 in {
----------------
jrtc27 wrote:
> isCodeGenOnly should be 0 and isAsmParserOnly should be 1, surely?
isAsmParserOnly = 1 would let it go into the tables in RISCVGenAsmMatcher.inc, but we don't want that here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108602