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

[Sam Elliott via Phabricator via llvm-commits]

lenary added a comment.

In D65205#1599133 <https://reviews.llvm.org/D65205#1599133>, @jrtc27 wrote:

> Yeah this looks like a nice way to do it, better than making an `InstAlias` for each one. GNU as will accept any expression that evaluates to the constant 0 in place of the 0 (i.e. it's just an imm0, same logic as all the other [su]immX fields, other than discarding it). Could we not therefore reuse part of the generic parser, just with a tweak to drop the immediate if parsed and 0? Something like:
>
> ... snip ...


I am worried about this approach, mostly because of the lines like 1268 in parseMemOpBaseReg, where they're pushing on `(`/`)` tokens, which aren't in the instruction operand strings (any more). It does seem like these might complicate the pushing/popping more than necessary. I am also not sure we want to support bare register operands.

That said, I do understand the value of using parseImmediate, to allow for any value that evaluates to zero.

> You could avoid the pushing and subsequent popping from the vector if you also split `RISCVAsmParser::parseImmediate` (and thus also `RISCVAsmParser::parseOperandWithModifier`) up into everything but the pushing.

This feels intrusive. At the moment I'm happy to refactor to using parseImmediate (but doing all my own register parsing to avoid paren issues), and I'll add a test to make sure it works with more complex expressions.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65205