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

[Jessica Clarke via Phabricator via llvm-commits]

jrtc27 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_}},
----------------
I guess MCK__40_ is because of `'('` in ASCII... not immediately obvious


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2326
+
+  // Attempt to parse token as a register.
+  if (parseRegister(Operands, true) == MatchOperand_Success)
----------------
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.


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2344
+/// parseDirectiveInsn
+/// ::= .insn [ format, encoding, (operands (, operands)*) ]
+bool RISCVAsmParser::parseDirectiveInsn(SMLoc L) {
----------------
Unfortunately it *doesn't* have that first comma, it's a bit ugly like that, though I can see why that was chosen


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2356
+
+  // Find entry for this format in InsnMatchTable.
+  auto EntryRange =
----------------
Plural?


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2428
+      // valid for a different match.
+      if ((it == EntryRange.first || ErrorInfo <= ActualIdx))
+        ErrorInfo = ActualIdx;
----------------
No assignment here, no double parens needed


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1340
 //===----------------------------------------------------------------------===//
+// .insn directive instructions
+//===----------------------------------------------------------------------===//
----------------
Hm, MC stuff is at the top, then CodeGen patterns+pseudos, not sure whether I like having them here or not, but I can see the argument for it, they're their own separate thing...


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1364
+
+let isCodeGenOnly = 1, hasSideEffects = 1, mayLoad = 1, mayStore = 1,
+    hasNoSchedulingInfo = 1 in {
----------------
isCodeGenOnly should be 0 and isAsmParserOnly should be 1, surely?


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.td:1369
+                                          AnyReg:$rs1, AnyReg:$rs2),
+                          ".insn r $opcode, $funct3, $funct7, $rd, $rs1, $rs2">;
+  def InsnR4 : DirectiveInsnR4<(outs), (ins uimm7:$opcode, uimm3:$funct3,
----------------
You could factor out the `".insn X"` bit into the directive's class so this just provides an argstr. Also do we want a tab anywhere in the string (either before or after the type) or not?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108602