Page MenuHomePhabricator

[RISCV] Add Custom Parser for Atomic Memory Operands

Authored by jrtc27 on Jul 25 2019, 9:27 AM.



GCC Accepts both (reg) and 0(reg) for atomic instruction memory
operands. These instructions do not allow for an offset in their
encoding, so in the latter case, the 0 is silently dropped.

Due to how we have structured the RISCVAsmParser, the easiest way to add
support for parsing this offset is to add a custom AsmOperand and
parser. This parser drops all the parens, and just keeps the register.

This commit also adds a custom printer for these operands, which matches
the GCC canonical printer, printing both (a0) and 0(a0) as (a0).

Based on D65205 by Sam Elliott.

Event Timeline

jrtc27 created this revision.Jul 25 2019, 9:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 9:27 AM

I've taken a look at this patch and am largely happy with it, except that I think it gives worse error messages than D65205 for invalid operands (compare llvm/test/MC/RISCV/rv64a-invalid.s and llvm/test/MC/RISCV/rv32a-invalid.s between the two patches).

@asb is currently away for a few days, but in discussions we've had, it seems likely we'll err towards merging D65205. This should happen soon, as we want this and the patch that depends on this backported to the 9.0 branch.

asb resigned from this revision.Aug 1 2019, 5:25 AM

Thanks for looking at an alternate approach here James. In light of the better error messages I'm going to go with D65205 at this point, but I'm open to future refactoring or improvements. I haven't spent a lot of time considering it, but my initial reaction on PushParens is I feel it might make the parsing logic harder to follow in return for a fairly minor reduction in repetition. But I could be convinced otherwise...

lenary added a comment.Aug 6 2019, 2:08 AM

Given D65205 has landed, please can you mark this patch as abandoned? That makes it easier to keep track of the RISC-V review pipeline. :)

jrtc27 abandoned this revision.Sep 3 2019, 2:23 AM

D65205 landed.