[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:2276
+     {MCK_UImm7, MCK_UImm3, MCK_UImm7, MCK_AnyReg, MCK_AnyReg, MCK_AnyReg}},
+    // Accept r with 4 registers as an alias for r4.
+    {"r",
----------------
Didn't notice this quirk the first time though but eww why was this deemed a good idea all those years ago...


================
Comment at: llvm/lib/Target/RISCV/AsmParser/RISCVAsmParser.cpp:2326
+
+  // Attempt to parse token as a register.
+  if (parseRegister(Operands, true) == MatchOperand_Success)
----------------
craig.topper wrote:
> 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?
No, that works because the cases where you have bare symbols that require context to know whether to parse a symbol or a register all use custom parsers, which are what MatchOperandParserImpl calls. Only after it's decided it's not a special case does it then return no match and fall back to the generic code like this.

... except it's currently broken for branches. For `j sym` we make sure to use SImm21Lsb0JAL, but for `beq reg, reg, sym` we don't use something with a custom parser, just a generic immediate operand. So I guess this does have feature parity after all. Now on my mental list of things to fix...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108602