Support capability atomics in hybrid mode #473

arichardson · 2020-09-08T17:26:25Z

This cleans up various atomic code generation bugs and adds support for atomic RMW operations in hybrid mode for both RISC-V and MIPS.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

arichardson · 2020-09-23T13:07:04Z

ping? (I'll fix the amdgpu test if this change looks ok)

clang/lib/CodeGen/CGAtomic.cpp

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td

arichardson · 2020-10-04T18:07:33Z

I've pushed fixup commits to make it easier to see the difference to the last version. Will squash them into the appropriate commit before merging if this looks good now.

arichardson · 2020-10-05T10:12:14Z

Last commit was submitted upstream as https://reviews.llvm.org/D88800

arichardson · 2020-10-09T14:17:39Z

ping?

jrtc27

I still don't like the fact that we're using cmpxchg for other AMOs unlike integer-in-normal/hybrid and capability-in-purecap atomics.

llvm/lib/CodeGen/AtomicExpandPass.cpp

clang/test/CodeGen/cheri/c11-atomic-caps.c

clang/lib/CodeGen/TargetInfo.cpp

clang/lib/CodeGen/CGAtomic.cpp

llvm/test/CodeGen/CHERI-Generic/RISCV32/atomic-rmw-cap-ptr.ll

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td

llvm/lib/CodeGen/AtomicExpandPass.cpp

The code was full of bugs that for some reason happened to generate working (or at least non-asserting) code in the majority of cases. However, when falling back to library calls for unsupported atomics arguments were not being passed correctly. As we don't implement those library calls this should not matter, but I was getting an assertion failure while compiling the atomics code from the latest QtBase dev branch. Now we call a non-existent libbcall instead of crashing which is a lot easier to debug. This also improves the -Watomic-alignment check to report unsupported rather than misaligned for operations such as xor/and/etc.

This fixes an assertion and incorrect generation of i128 IR instead of i8 addrspace(200)* when generating hybrid MIPS C11 atomics. It turns out that we are using libcalls even for load/store/xchg for MIPS which will be fixed in a follow-up commit.

This matches RISC-V and allows reuse of the c11-atomic-caps.c checks for CHERI-RISC-V.

This fixes the atomicexpandpass to always emit pointer type cmpxchg instructions and adds missing some instruction selection patterns that are required now that MIPS no longer uses libcalls for some atomics. Note: RISC-V falls back to atomic libcalls but selects the generic overload which cannot handle capabilities correctly.

We need to call a specialized libcall for capability arguments since the generic one will not handle capability tag bits. Note: we don't have an implementation of those spciailized functions but a linker error is much better than silent run-time tag corruption.

Also add a test that all RMW operations generate sensible code in both purecap and hybrid mode when passed a capability pointer (and a non-capability argument). This test shows that the same IR will result in an assertion for RISC-V which was already reported as CTSRD-CHERI#470

I don't think this can currently be triggered, but I found it while testing some changes to the CHERI-RISC-V instruction definitions.

Previously we would assert when attempting to perform an atomic RMW operation using a capability pointer operand. This change implements atomic RMW operations using an integer argument by expanding them to a cmpxchg sequence (which results in a lr.<size>.cap+sc.<size>.cap loop). The generated code could be made more efficient by implementing the necessary hooks for AtomicExpansionKind::LLSC, but that can be done later. Fixes CTSRD-CHERI#470

arichardson · 2020-11-03T15:27:05Z

You can ignore all commits other than the last two that don't start with fixup: since they are the same as the last version.

In preparation for the next commit. No functional change but makes bugs easier to spot.

This change adds new intrinsics that are custom-lowered to the explicit mode intrinsics since writing tablegen patterns for all the variations would be extremely tedious (and also result in difficult-to-debug failure modes).

arichardson · 2020-11-04T12:54:39Z

Updated to use LR/SC now.

This makes it more awkward to use for out CHERI-RISC-V atomics with explicit addressing mode since the RISC-V backend only treats XLen-sized integers as legal. Since this value is only used in an icmp, any integer type should be fine and it allows us to return an i64 for RV64 instead.

This should generate the same code as i8 addrspace(200)*.

llvm/test/CodeGen/CHERI-Generic/Inputs/atomic-rmw-cap-ptr-cap-arg.ll

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

jrtc27 · 2020-11-08T23:20:55Z

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

+  // The atomic RMW instructions are only available in capmode, so for the
+  // hybrid ABI we have expand these operations using a LR/SC sequence.
+  if (needsExplicitAddressingModeAtomics(AI, Subtarget))
+    return AtomicExpansionKind::LLSC;


This still is differing. It should be doing things exactly the same as the non-explicit versions, just with different expansions in ExpandAtomicPseudoInsts. No new intrinsics should be needed, just a bunch of SelectionDAG pseudos and patterns mirroring what's already there.

Seems like quite a lot of effort. Maybe I'll get to it next year

FWIW AtomicExpansionKind::LLSC is particularly broken, as it's done far too early in CodeGen so spills and reloads can sneak in between the two, which is highly problematic on architectures that permit very simple implementations with a single link/reserved bit. RISC-V's forward-progress guarantee requires the code to not use any memory instructions in between the LR and the SC, among other things. This is why the pseudos exist and are expanded so late, otherwise RISC-V would just use the existing expansion code and save a lot of effort. CmpXChg does work but is of course much less efficient when your primitive is LR/SC.

Good to know, I assumed it would work considering that ARM/AArch64 uses it.

The Arm ARM probably mandates a less-stupid implementation, although maybe technically if you put an atomic on the stack you can get issues there, but why on earth would you do that.

(See https://bugs.llvm.org/show_bug.cgi?id=32020 for a real-world case of this mattering)

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

jrtc27 · 2020-11-08T23:23:42Z

llvm/lib/Target/RISCV/RISCVISelLowering.cpp

@@ -3440,14 +3531,6 @@ Value *RISCVTargetLowering::emitMaskedAtomicCmpXchgIntrinsic(
  return Result;
 }

-bool RISCVTargetLowering::canLowerPointerTypeCmpXchg(
-    const llvm::DataLayout &DL, llvm::AtomicCmpXchgInst *AI) const {
-  if (Subtarget.hasStdExtA() && Subtarget.hasCheri() &&


Does deleting the A check not break non-A codegen?

As you can see in llvm/test/CodeGen/CHERI-Generic/RISCV64/cmpxchg-cap-ptr.ll, removing a results in libcalls, so it does not seem to be required.
If I recall correctly, this function is only used to determine whether the IR can contain cmpxchg instructions with pointer types instead of just integers (since non-RISC-V/non-MIPS backends expect integer operands).

llvm/test/CodeGen/CHERI-Generic/Inputs/atomic-rmw-cap-ptr-cap-arg.ll

llvm/test/CodeGen/RISCV/cheri/fp32-load-store-hybrid.ll

llvm/test/CodeGen/RISCV/cheri/fp64-load-store-hybrid.ll

jrtc27 · 2020-11-08T23:37:03Z

llvm/lib/CodeGen/AtomicExpandPass.cpp

@@ -1770,7 +1800,7 @@ bool AtomicExpand::expandAtomicOpToLibcall(
  auto PtrTypeAS = PointerOperand->getType()->getPointerAddressSpace();
  Value *PtrVal = Builder.CreateBitCast(PointerOperand,
                                        Type::getInt8PtrTy(Ctx, PtrTypeAS));
-  unsigned PtrValAS = DL.getAllocaAddrSpace();
+  auto PtrValAS = PointerOperandIsCap ? PtrTypeAS : DL.getAllocaAddrSpace();


When would this ever not be just PtrTypeAS?

I think this could happen for other architectures, where you can only pass one type of pointer to libfuncs. We can pass both AS0 and AS200, but that might not always be possible.

jrtc27 · 2020-11-09T13:47:10Z

Can we split out frontend, target-independent and MIPS bits though, so it's just a case of someone bothering to do the RISC-V backend grunt work?

This current results in a selection failure for RISCV when used on i8/i16 since it's trying to use the masked intrinsics.

… mode This was previously asserting since the backend tried to use the masked instrinsics (which cannot be used with capabilities and also should not be used since we need to remain in-bounds).

… hybrid mode

Also drop the unnncessary fadd. I think I originally added this to double- check that it was really generating soft-float code, but this can also be seen by the lack of fmv.x.w instructions.

arichardson · 2020-11-09T14:15:31Z

Will split this up into smaller parts.

arichardson · 2020-11-12T13:49:51Z

Abadoning this PR in favour of smaller ones.

arichardson marked this pull request as draft September 8, 2020 17:26

jrtc27 reviewed Sep 8, 2020

View reviewed changes

llvm/lib/Target/RISCV/RISCVISelLowering.cpp Outdated Show resolved Hide resolved

arichardson force-pushed the hybrid-atomics branch from a39a0c4 to b524fc8 Compare September 9, 2020 15:27

jrtc27 reviewed Sep 23, 2020

View reviewed changes

clang/lib/CodeGen/CGAtomic.cpp Outdated Show resolved Hide resolved

jrtc27 reviewed Sep 23, 2020

View reviewed changes

llvm/lib/Target/RISCV/RISCVISelLowering.cpp Outdated Show resolved Hide resolved

jrtc27 reviewed Sep 23, 2020

View reviewed changes

llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td Show resolved Hide resolved

arichardson marked this pull request as ready for review September 23, 2020 14:13

arichardson requested a review from jrtc27 October 4, 2020 18:07

jrtc27 reviewed Nov 1, 2020

View reviewed changes

arichardson added 4 commits November 3, 2020 11:54

fixup: regen with new update_cc_test_checks

0fce54a

fixup: improve comment.

7136283

Fix c11-atomic-caps.c for MIPS hybrid

b0100a5

This fixes an assertion and incorrect generation of i128 IR instead of i8 addrspace(200)* when generating hybrid MIPS C11 atomics. It turns out that we are using libcalls even for load/store/xchg for MIPS which will be fixed in a follow-up commit.

arichardson force-pushed the hybrid-atomics branch from f44ba4e to 8951c74 Compare November 3, 2020 15:17

arichardson added 12 commits November 3, 2020 15:25

fixup: use intcaptype instead of unsigned intcaptype

fd2f957

Add CHECK lines for RISCV64 hybrid to c11-atomic-caps.c

9052292

[CHERI-MIPS][hybrid] Stop emitting libcalls for simple atomic ops

76fa777

This matches RISC-V and allows reuse of the c11-atomic-caps.c checks for CHERI-RISC-V.

fixup: drop unncessary comment

94174cf

Add nounwind to make test a bit shorter. NFC

63f5a57

fixup: indentation (clang-format got it wrong)

45bfb41

[RISC-V] AsmParser: avoid crashing on invalid tied operand

3e848fd

I don't think this can currently be triggered, but I found it while testing some changes to the CHERI-RISC-V instruction definitions.

fixup: style fixes

7c1efea

arichardson force-pushed the hybrid-atomics branch from 8951c74 to f14898b Compare November 3, 2020 15:26

arichardson added 3 commits November 3, 2020 17:23

[CHERI] Add tests for atomic capability ops using non-sc memory order

540e917

[CHERI-Generic] Update atomics test to use the return values

eb295be

In preparation for the next commit. No functional change but makes bugs easier to spot.

arichardson force-pushed the hybrid-atomics branch from 15a36f9 to 8806d23 Compare November 4, 2020 13:03

arichardson added 4 commits November 4, 2020 14:18

[CHERI-RISC-V] Avoid unncessary trunc when expanding explicit atomics

9398b40

[CHERI-Generic] Add a test for atomicrmw xchg i32 addrspace(200)*

f6ecb7e

This should generate the same code as i8 addrspace(200)*.

[NFC] Minor simplification of AtomicInfo::emitCastToAtomicIntPointer()

c0f9cfa

arichardson force-pushed the hybrid-atomics branch from 8806d23 to 38f7b1c Compare November 4, 2020 14:18

arichardson requested a review from jrtc27 November 4, 2020 14:19

arichardson commented Nov 4, 2020

View reviewed changes

llvm/test/CodeGen/CHERI-Generic/Inputs/atomic-rmw-cap-ptr-cap-arg.ll Outdated Show resolved Hide resolved

jrtc27 reviewed Nov 8, 2020

View reviewed changes

arichardson marked this pull request as draft November 9, 2020 13:36

arichardson added 6 commits November 9, 2020 13:57

Add a test for cmpxchg using capability pointers in hybrid mode

87d2498

This current results in a selection failure for RISCV when used on i8/i16 since it's trying to use the masked intrinsics.

[CHERI-RISC-V] Fix cmpxchg with capability pointer operands in hybrid…

0b1dd96

… mode This was previously asserting since the backend tried to use the masked instrinsics (which cannot be used with capabilities and also should not be used since we need to remain in-bounds).

fixup: [CHERI-RISC-V] Handle capability-based atomic RMW in hybrid mode

59204d4

fixup: [CHERI-RISC-V] Fix cmpxchg with capability pointer operands in…

b8b0166

… hybrid mode

fixup: rename double/float tests

0a68178

Add purecap tests to float-mem.ll and double-mem.ll

e60382c

Also drop the unnncessary fadd. I think I originally added this to double- check that it was really generating soft-float code, but this can also be seen by the lack of fmv.x.w instructions.

arichardson force-pushed the hybrid-atomics branch from 38f7b1c to e60382c Compare November 9, 2020 14:24

This was referenced Nov 11, 2020

Hybrid float via cap #488

Merged

Hybrid atomics fixes (part 1) #489

Merged

arichardson closed this Nov 12, 2020

arichardson mentioned this pull request Nov 12, 2020

[CHERI-RISCV] Inline expansion of capability-based cmpxchg in hybrid mode #490

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support capability atomics in hybrid mode #473

Support capability atomics in hybrid mode #473

arichardson commented Sep 8, 2020 •

edited

Loading

arichardson commented Sep 23, 2020

arichardson commented Oct 4, 2020

arichardson commented Oct 5, 2020

arichardson commented Oct 9, 2020

jrtc27 left a comment

arichardson commented Nov 3, 2020

arichardson commented Nov 4, 2020

jrtc27 Nov 8, 2020

arichardson Nov 9, 2020

jrtc27 Nov 9, 2020 •

edited

Loading

arichardson Nov 9, 2020

jrtc27 Nov 9, 2020 •

edited

Loading

jrtc27 Nov 9, 2020

jrtc27 Nov 8, 2020

arichardson Nov 9, 2020

jrtc27 Nov 8, 2020

arichardson Nov 9, 2020

jrtc27 commented Nov 9, 2020

arichardson commented Nov 9, 2020

arichardson commented Nov 12, 2020

Support capability atomics in hybrid mode #473

Support capability atomics in hybrid mode #473

Conversation

arichardson commented Sep 8, 2020 • edited Loading

arichardson commented Sep 23, 2020

arichardson commented Oct 4, 2020

arichardson commented Oct 5, 2020

arichardson commented Oct 9, 2020

jrtc27 left a comment

Choose a reason for hiding this comment

arichardson commented Nov 3, 2020

arichardson commented Nov 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 Nov 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jrtc27 commented Nov 9, 2020

arichardson commented Nov 9, 2020

arichardson commented Nov 12, 2020

arichardson commented Sep 8, 2020 •

edited

Loading

jrtc27 Nov 9, 2020 •

edited

Loading

jrtc27 Nov 9, 2020 •

edited

Loading