[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:2326
+
+  // Attempt to parse token as a register.
+  if (parseRegister(Operands, true) == MatchOperand_Success)
----------------
jrtc27 wrote:
> 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...
Yeah I saw we used the custom parser for `j sym` so I matched it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108602