This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Codegen support for atomic operations on RV32I
ClosedPublic

Authored by asb on May 31 2018, 6:44 AM.

Details

Summary

This patch adds lowering for atomic fences to the native RV32I fence instructions and relies on AtomicExpandPass to lower atomic loads/stores, atomic rmw, and cmpxchg to __atomic_* libcalls. Future patches add support for RV32A, providing atomics support without libcalls.

test/CodeGen/RISCV/atomic-* are modelled on the exhaustive test/CodeGen/PPC/atomics-regression.ll, and will prove more useful once RV32A codegen support is introduced. I'd welcome feedback on if I'm missing anything obvious. I believe that lowering fences but emitting __atomic libcalls otherwise seems to match the behaviour of other backends (e.g. Sparc) and seems correct, but I'd appreciate further opinions.

Fence mappings are taken from table A.6 in the current draft of version 2.3 of the RISC-V Instruction Set Manual, which incorporates the memory model changes and definitions contributed by the RISC-V Memroy Consistency Model task group.

Diff Detail

Repository
rL LLVM

Event Timeline

asb created this revision.May 31 2018, 6:44 AM
asb edited the summary of this revision. (Show Details)May 31 2018, 6:46 AM
jfb added inline comments.May 31 2018, 8:47 AM
lib/Target/RISCV/RISCVInstrInfo.td
731 ↗(On Diff #149280)

The table says this should be rw, w.

732 ↗(On Diff #149280)

Can you define fence.tso first?

asb updated this revision to Diff 149330.May 31 2018, 11:49 AM
asb marked 2 inline comments as done.

Update to address comments from @jfb (thanks!).

jfb added a comment.May 31 2018, 12:42 PM

Looks fine in general, though I know ~nothing of RISCV.

lib/Target/RISCV/RISCVISelLowering.cpp
145 ↗(On Diff #149330)

Will you always want libcalls, or is it just a temporary thing. Seems you want a comment to explain.

test/CodeGen/RISCV/atomic-fence.ll
27 ↗(On Diff #149330)

You should comment about fence.tso here as well.

asb updated this revision to Diff 150324.Jun 7 2018, 7:02 AM
asb marked 2 inline comments as done.

Update to address all outstanding review comments.

I've also removed part-world (i8/i16) atomicrmw min/max/umin/umax from atomic-rmw.ll, as they will not be supported in the first round of RV32A patches and are non-standard (Clang will refuse to generate them, and they're unsupported on other archs like Mips).

asb added inline comments.Jun 7 2018, 8:31 AM
lib/Target/RISCV/RISCVInstrInfo.td
731 ↗(On Diff #149280)

Fixed the comment, thanks.

732 ↗(On Diff #149280)

I'll clarify the comment. The memory model working group proposed that a new fence.tso instruction should be added to the base instruction set, but that proposal hasn't been accepted and even if it was we'd of course have to be very careful about emitting fence.tso for hardware that doesn't actually implement it.

fence.tso is in the draft spec you linked to -- are you not using it because it hasn't been finalized, or was it just added after you left that comment?

test/CodeGen/RISCV/atomic-rmw.ll
495–496 ↗(On Diff #150324)

I think we ought to support these -- they're not much harder to implement than nand, I think. But I'll leave a comment on the other patch.

asb added a comment.Jun 7 2018, 12:49 PM

fence.tso is in the draft spec you linked to -- are you not using it because it hasn't been finalized, or was it just added after you left that comment?

Thanks for querying. I actually had been referring to an earlier draft. But even with the current draft I'm concerned that v2.0 of the ISA manual didn't specify that instr[31:28] should be ignored, meaning fence.tso may simply trap on some implementations. I'll query if anyone is tracking this behaviour across available RISC-V IP cores.

test/CodeGen/RISCV/atomic-rmw.ll
495–496 ↗(On Diff #150324)

Not hard, just require another branch+BB and maybe an extra scratch register (there's no conditional move in RISC-V). So it does complicate the atomics lowering somewhat. But yes, it does feel like we should aim for completeness.

asb added a comment.Jun 7 2018, 1:05 PM
In D47587#1125518, @asb wrote:

Thanks for querying. I actually had been referring to an earlier draft. But even with the current draft I'm concerned that v2.0 of the ISA manual didn't specify that instr[31:28] should be ignored, meaning fence.tso may simply trap on some implementations. I'll query if anyone is tracking this behaviour across available RISC-V IP cores.

Scratch that. Version 2.0 of the spec did indicate that implementations should ignore the zeroed fields, except it did so in an aside rather than the main text. I'll update to define and make use of fence.tso. Given the poor state of compliance testing in the RISC-V world there is some chance this will cause problems on some implementations despite what the spec says.

asb updated this revision to Diff 150479.Jun 8 2018, 3:55 AM

Update to use fence.tso for fence acq_rel.

jyknight accepted this revision.Jun 8 2018, 10:33 AM
jyknight added inline comments.
lib/Target/RISCV/RISCVInstrInfo.td
756 ↗(On Diff #150479)

Did you miss uploading the part of the change to add a definition of FENCE_TSO? I don't see where that's defined here.

But marking LGTM anyways, assuming that'll get fixed before commit.

This revision is now accepted and ready to land.Jun 8 2018, 10:33 AM
asb updated this revision to Diff 151125.Jun 13 2018, 4:55 AM

Update to add atomicrmw max/min/umax/umin to tests.

asb added inline comments.Jun 13 2018, 4:58 AM
lib/Target/RISCV/RISCVInstrInfo.td
756 ↗(On Diff #150479)

I committed MC layer support for fence.tso directly (i.e. with post-commit review). MC layer fence.tso support is logically independent of this patch, and the patch was trivial. See rL334278.

This revision was automatically updated to reflect the committed changes.
In D47587#1125544, @asb wrote:
In D47587#1125518, @asb wrote:

Thanks for querying. I actually had been referring to an earlier draft. But even with the current draft I'm concerned that v2.0 of the ISA manual didn't specify that instr[31:28] should be ignored, meaning fence.tso may simply trap on some implementations. I'll query if anyone is tracking this behaviour across available RISC-V IP cores.

Scratch that. Version 2.0 of the spec did indicate that implementations should ignore the zeroed fields, except it did so in an aside rather than the main text. I'll update to define and make use of fence.tso. Given the poor state of compliance testing in the RISC-V world there is some chance this will cause problems on some implementations despite what the spec says.

As expected... AllWinner-D1 (Risc-V 64 single core) does not implement fence.tso and will raise SIGILL...

In D47587#1125544, @asb wrote:
In D47587#1125518, @asb wrote:

Thanks for querying. I actually had been referring to an earlier draft. But even with the current draft I'm concerned that v2.0 of the ISA manual didn't specify that instr[31:28] should be ignored, meaning fence.tso may simply trap on some implementations. I'll query if anyone is tracking this behaviour across available RISC-V IP cores.

Scratch that. Version 2.0 of the spec did indicate that implementations should ignore the zeroed fields, except it did so in an aside rather than the main text. I'll update to define and make use of fence.tso. Given the poor state of compliance testing in the RISC-V world there is some chance this will cause problems on some implementations despite what the spec says.

As expected... AllWinner-D1 (Risc-V 64 single core) does not implement fence.tso and will raise SIGILL...

That'll have to be patched up in M-mode then. I guess they only tested with GCC, which lowers atomic_thread_fence(memory_order_acq_rel) to an extremely overly strong fence iorw, iorw. I believe that's the only way to get a fence.tso currently; atomic reads/writes don't permit memory_order_acq_rel, and the read-modify-writes that do just use .aqrl variants of the instructions.

llvm/trunk/test/CodeGen/RISCV/atomic-load-store.ll