[PATCH] D65205: [RISCV] Add Custom Parser for Atomic Memory Operands

[James Clarke via Phabricator via llvm-commits]

jrtc27 added a comment.

In D65205#1600772 <https://reviews.llvm.org/D65205#1600772>, @lenary wrote:

> > Is there a reason not to include them in the instruction operand string? It would mean you don't need the custom printer, no?
>
> This is actually really complex. If I include the parens in the tablegen instruction definition, then we definitely cannot support `0(a0)` because the parser requires a `(` where the `0` is. If i don't include them in the instruction definition, then when parseMemBaseOp runs, it adds `(` tokens into the Operands, which means later the register operand is not in the expected place in Operands when operand matching happens.


Yeah, I see the problem now, we can't trigger our custom parser early enough.

>> Yes, that's another sensible alternative.
> 
> Update: I looked into doing this, and it's a lot more complex than I hoped. parseImmediate accepts left parens, and we cannot backtrack through a parse. So, if you write `lr.w a0, (a1)`, parseImmediate tries to parse `(a1)` as an immediate, which succeeds, and then only fails when you try to check it's a constant. I would therefore have to duplicate my memory operand parsing code (which drops parens, because above) to attempt to parse a register first, and then if that fails, backtrack and attempt to parse an immediate then a register. There are other approaches (doing lookahead, and assuming that `(<token>)` must refer to a register, so skipping the immediate parsing if the third token is a right paren) but they all feel really hacky and not wonderful.
> 
> For the moment I'm going to leave the code approximately* how it is currently, because I do not see the added value in the extra complexity by using parseImmediate, compared to the added assembly constructs (computed offsets) that it supports.
> 
> *= I may have a more robust way of checking the immediate is zero, based on your code and other `isImm*` functions, which I want to polish and add to this patch.

I personally don't think it's too bad, and created D65291 <https://reviews.llvm.org/D65291> based on this to demonstrate it. I will let others decide which they prefer though.



================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoA.td:19
+// A parse method for (${gpr}) or 0(${gpr}), where the 0 is be silently ignored.
+// Used for GCC Compatibility.
+def AtomicMemOpOperand : AsmOperandClass {
----------------
GNU as, not GCC


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65205