[PATCH] D54296: [RISCV] Lower inline asm constraint A for RISC-V

[James Clarke via Phabricator via llvm-commits]

jrtc27 added inline comments.


================
Comment at: lib/Target/RISCV/RISCVAsmPrinter.cpp:118
 
-    OS << "0(" << RISCVInstPrinter::getRegisterName(MO.getReg()) << ")";
+    OS << "(" << RISCVInstPrinter::getRegisterName(MO.getReg()) << ")";
     return false;
----------------
jrtc27 wrote:
> asb wrote:
> > Why make this change? It doesn't seem to match the GNU tools default, which I'd rather we stay in line with.
> > 
> > ```
> > $ cat t.s 
> > lw a0, 0(a0)
> > $ ./riscv32-unknown-elf-as t.s
> > $ ./riscv32-unknown-elf-objdump -d a.out 
> > 
> > a.out:     file format elf32-littleriscv
> > 
> > 
> > Disassembly of section .text:
> > 
> > 00000000 <.text>:
> >    0:	00052503          	lw	a0,0(a0)
> > ```
> Because we can’t differentiate between A and m constraints here. A has no immediate and is suitable for all loads and stores, including atomics, so GCC will emit just `(reg)` matching the canonical form for atomics (GNU as also accepts an immediate that evaluates to zero and discards it). Whereas m can have an immediate, though we hard-coded it to 0 here. We accept `lw x1, (x2)` but we don’t accept (or at least certainly didn’t used to accept) `lr.w x1, 0(x2)`, so we have to emit the form that works for everything, even if it’s not canonical for immediate loads and stores.
Ok, I was wrong, GCC will actually emit it as `0(reg)`, which only works because GNU as accepts `lr.w rd, 0(rs1)` despite not taking an immediate. We have two options:

1. Emit `(rs1)` (this patch) and rely on the existing `lw rd, (rs1)` aliases
2. Emit `0(rs1)` (current behaviour) and add new `lr.w rd, 0(rs1)`/`sc.w rd, rs2, 0(rs1)`/`amo rd, rs2, 0(rs1)` aliases


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54296