Page MenuHomePhabricator

[RISCV] Lower inline asm constraint A for RISC-V
ClosedPublic

Authored by lewis-revill on Nov 9 2018, 1:25 AM.

Details

Reviewers
asb
Summary

This allows arguments with the constraint A to be lowered to input nodes for RISC-V, which implies a memory address stored in a register.

This patch adds the minimal amount of code required to get operands with the right constraints to compile.

Diff Detail

Repository
rL LLVM

Event Timeline

lewis-revill created this revision.Nov 9 2018, 1:25 AM

Don't output an additional '0' when printing indirect memory operands.

Herald added a project: Restricted Project. · View Herald TranscriptFeb 5 2019, 7:44 AM
jrtc27 added a comment.Feb 5 2019, 4:34 PM

I think we should introduce a new Constraint_A enum member for this case. Whilst "A" currently behaves the same for the RISC-V backend as "m", that won't necessarily be the case forever. "A" is required to always be a single GPR (and so can be used for the atomic instructions, or ones with reduced immediate encoding space), but "m" (at least according to GCC) is "any kind of address that the machine supports in general", which I assume for RISC-V means reg+simm12.

lewis-revill retitled this revision from [WIP, RISCV] Lower inline asm constraint A for RISC-V to [RISCV] Lower inline asm constraint A for RISC-V.

Add Constraint_A to account for future modifications to Constraint_m handling. Constraint_A should only ever utilize a single base address without an offset.

asb requested changes to this revision.EditedJun 18 2019, 8:09 AM

Added a couple of comments. Once addressed, should be fine. Thanks!

lib/Target/RISCV/RISCVAsmPrinter.cpp
118 ↗(On Diff #204506)

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)
lib/Target/RISCV/RISCVISelDAGToDAG.cpp
182

Doesn't need to be an extra case given the body is the same as the others.

This revision now requires changes to proceed.Jun 18 2019, 8:09 AM
jrtc27 added inline comments.Jun 18 2019, 8:29 AM
lib/Target/RISCV/RISCVAsmPrinter.cpp
118 ↗(On Diff #204506)

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.

lewis-revill added inline comments.Jun 19 2019, 3:29 AM
lib/Target/RISCV/RISCVISelDAGToDAG.cpp
182

I added this to differentiate between the 'm' and 'A' because of the possibility of more complex 'm' constraints like @jrtc27 mentioned, IE including both an offset and a base.

jrtc27 added inline comments.Jul 1 2019, 9:32 AM
lib/Target/RISCV/RISCVAsmPrinter.cpp
118 ↗(On Diff #204506)

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
lenary added a subscriber: lenary.Jul 22 2019, 2:34 AM
lenary added inline comments.
lib/Target/RISCV/RISCVAsmPrinter.cpp
118 ↗(On Diff #204506)

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.

lewis-revill added inline comments.Jul 22 2019, 3:28 AM
lib/Target/RISCV/RISCVAsmPrinter.cpp
118 ↗(On Diff #204506)

Ok, will update this patch once this is done.

lenary added inline comments.Jul 24 2019, 6:21 AM
lib/Target/RISCV/RISCVAsmPrinter.cpp
118 ↗(On Diff #204506)

Patch posted: D65205. I also added it as a parent patch.

Rebased and updated to use '0(' prefix for memory constraints.

asb accepted this revision.Aug 1 2019, 5:11 AM

Looks good to me - thanks!

This revision is now accepted and ready to land.Aug 1 2019, 5:11 AM
lewis-revill edited the summary of this revision. (Show Details)

Rebased prior to commit.

lewis-revill closed this revision.Aug 16 2019, 3:39 AM

Committed (ill formed commit message so phabricator did not pick it up)