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

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added inline comments.


================
Comment at: lib/Target/RISCV/RISCVAsmPrinter.cpp:118
 
-    OS << "0(" << RISCVInstPrinter::getRegisterName(MO.getReg()) << ")";
+    OS << "(" << RISCVInstPrinter::getRegisterName(MO.getReg()) << ")";
     return false;
----------------
lenary wrote:
> jrtc27 wrote:
> > 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
> I've talked this over with @asb, and we're going to go with option 2. I'm working on a patch for that this morning, I will add it as a parent patch to this once it's ready. 
Ok, will update this patch once this is done.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54296