Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
wants to merge 8 commits into
base: dev
Choose a base branch
from
Open

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

wants to merge 8 commits into from
+4,538 −1,054

Conversation

arichardson
Copy link
Member

@arichardson arichardson commented Nov 12, 2020 •
edited

Factored out from #473 and rebased on top of #491

@arichardson arichardson force-pushed the hybrid-atomics-part2 branch 2 times, most recently from 8053c29 to 1f423d3 Compare December 2, 2020 12:45
@jrtc27
Copy link
Member

jrtc27 commented Dec 7, 2020

Haven't had a chance to look at it properly yet, but could you please hoist the CheriStoreCond_r operand before all the other commits (and add the "-" in RISC-V while you're at it)?

@arichardson arichardson force-pushed the hybrid-atomics-part2 branch 2 times, most recently from fb79cca to fe93bf8 Compare December 8, 2020 22:19
@arichardson
Copy link
Member Author

The last commit now adds support for RMW ops, I can split that out into a separate PR if you'd prefer.

@arichardson
Copy link
Member Author

rebased and updated tests for each commit.

@arichardson arichardson force-pushed the hybrid-atomics-part2 branch 2 times, most recently from 795f797 to c29c0c9 Compare October 15, 2021 21:08
@arichardson
Copy link
Member Author

ping? I spent quite a bit of time on this a few months ago so it would be nice to get it merged.

jrtc27
jrtc27 reviewed Oct 24, 2021
View reviewed changes
llvm/lib/Target/RISCV/RISCVISelLowering.cpp
// instead of promoting to i32/i64 to ensure we don't trigger bounds errors.
const DataLayout &DL = CI->getModule()->getDataLayout();
uint64_t Size =
DL.getTypeSizeInBits(CI->getCompareOperand()->getType()).getFixedSize();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the difference between these two? Shouldn't we always have a primitive here whose size we can get? The code on the left comes from upstream, and there are a bunch of other places that use that form too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the former does not handle pointers since it's not got a DL.

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td Show resolved Hide resolved
llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td Outdated Show resolved Hide resolved
llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td Outdated Show resolved Hide resolved
llvm/test/CodeGen/CHERI-Generic/RISCV32/cmpxchg-cap-ptr.ll Outdated
; HYBRID-ATOMICS-NEXT: .LBB0_3: # %partword.cmpxchg.end
; HYBRID-ATOMICS-NEXT: xor a0, a5, a0
; HYBRID-ATOMICS-NEXT: seqz a1, a0
; HYBRID-ATOMICS-NEXT: srl a0, a5, a6
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Masked atomics are so sad... stupid RISC-V. Though this codegen looks rather different to (and worse than) plain RISC-V on integers.

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
const Value *Ptr) const {
if (I->getModule()->getDataLayout().isFatPointer(Ptr->getType()))
return 8; // All sizes are supported via capabilities
return TargetLowering::getMinCmpXchgSizeInBits(I, Ptr);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How come integer pointers work fine for RISC-V then?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if I understand the question, but i8/i16 ops are expanded in IR: https://godbolt.org/z/Y9bKxeb88

llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td Outdated Show resolved Hide resolved
llvm/lib/Target/RISCV/RISCVISelLowering.cpp Outdated Show resolved Hide resolved
llvm/lib/Target/RISCV/RISCVInstrInfoXCheri.td Show resolved Hide resolved
@arichardson
[CHERI-RISC-V] Swap CheriStoreCond_r operand order to match AMO_rr
8e402bc
@arichardson
[CHERI-RISC-V] Fix capability i8/i16 cmpxchg in hybrid mode
71e651f 
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).
The test for this commit will be included in the next one since this
depends on cmpxchg being expanded.
@arichardson
[CHERI-RISC-V] Handle capability-based cmpxchg in hybrid mode
140f9fa 
This adds the required SelectionDAG nodes and RISCVExpandAtomicPseudoInsts
changes to emit cmpxchg (lr.cap/sc.cap) in hybrid mode.

There is one known issues which will be fixed in the next commits:
All explicit mode atomics are relaxed, so we are missing fences.
@arichardson
[CHERI-RISC-V] Fix lowering of i8/i16 cmpxchg using a capability pointer
16ab419 
We should not be using the expandPartwordCmpXchg() logic since capability-
based atomics have i8 and i16 variants. To fix this, add a Type argument
to TLI->getMinCmpXchgSizeInBits() so that the RISC-V backend can return
8 for capability-based cmpxchg and 32 for non-capability ones.
@arichardson
[CHERI-Generic] Add cmpxchg tests with all memory order combinations
3285480 
This is in preparation for the next commit since they are still missing
the required fences for RISC-V.
@arichardson
[CHERI-RISC-V] Insert the required fences for explicit address mode a…
e3d507c 
…tomics

The explicit addressing mode atomics always use relaxed ordering so we
need to insert fences if strong orderings are requested.
Fortunately there is already support for this in the AtomicExpandPass so
all we need to do here is to fix emitLeadingFence/emitTrailingFence and
handle RMW instructions in shouldInsertFencesForAtomic().
@arichardson
[CHERI-RISC-V] Support capability-based integer atomic RMW ops
04c69db 
This allows expanding all integer atomic RMW operations using capability
pointers inline in hybrid mode.
@arichardson
[CHERI-RISC-V] Expand capability AtomicRMW XCHG in hybrid mode
ef40de6 
This allows expanding e.g. `atomicrmw xchg i32 addrspace(200)*` without a
library call. Code generation could be improved by adding an explicit
pseudo but that can be done as a follow-up change
@arichardson
Copy link
Member Author

rebased and hopefully addressed all comments.

@arichardson arichardson mentioned this pull request Sep 22, 2023
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jrtc27 jrtc27 jrtc27 left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@arichardson @jrtc27