This is an archive of the discontinued LLVM Phabricator instance.

[PATCH] Improve support for atomicrmw and cmpxchg in C API.
ClosedPublic

Authored by nickwasmer on Sep 3 2019, 3:36 PM.

Details

Summary

atomicrmw and cmpxchg have a volatile flag, so allow them to be get and set with LLVM{Get,Set}Volatile. atomicrmw and fence have orderings, so allow them to be get and set with LLVM{Get,Set}Ordering. Add missing LLVMAtomicRMWBinOpFAdd and LLVMAtomicRMWBinOpFSub enum constants. AtomicCmpXchg also has a weak flag, add a getter/setter for that too. Add a getter/setter for the binary-op of an atomicrmw.

atomicrmw and cmpxchg have a volatile flag, so allow it to be set/get with LLVMGetVolatile and LLVMSetVolatile. Add missing LLVMAtomicRMWBinOpFAdd and LLVMAtomicRMWBinOpFSub enum constants. AtomicCmpXchg also has a weak flag, add a getter/setter for that too. Add a getter/setter for the binary-op of an atomicrmw.

While fixing atomics, I noticed that LLVMIsA## was missing for CatchSwitchInst, CallBrInst and FenceInst, as well as AtomicCmpXchgInst and AtomicRMWInst. Fixed them all.

Update llvm-c-test to include atomicrmw and fence, and to copy volatile for the four applicable instructions.

Diff Detail

Repository
rL LLVM

Event Timeline

nickwasmer created this revision.Sep 3 2019, 3:36 PM
Herald added a project: Restricted Project. · View Herald Transcript
nickwasmer updated this revision to Diff 219164.Sep 6 2019, 1:49 PM

Add the ability to update ordering on atomicrmw and fence.

nickwasmer edited the summary of this revision. (Show Details)Sep 6 2019, 1:51 PM

Ping! It's been 1 week since I sent out this patch.

Here's my suggestion for how to review the patch which you may want to follow:
a) check the change to AtomicRMWBinOp. The list should match AtomicRMWInst::BinOp link
b) check mapFromLLVMRMWBinOp and mapToLLVMRMWBinOp, ensure they cover all cases and are each other's inverses.
c) the inclusion of CatchSwitchInst, CallBrInst, AtomicCmpXchgInst, AtomicRMWInst and FenceInst in LLVM_FOR_EACH_VALUE_SUBCLASS. This has the effect is creating LLVMIsACatchSwitchInst(LLVMValueRef) methods, but little else (nothing else?).
d) LLVMGetVolatile, LLVMSetVolatile, LLVMGetOrdering and LLVMSetOrdering. Compare with the LangRef to make sure those are the only values which have volatile, and the only values which have ordering (note: not including cmpxchg which has two orderings and already has its own C API methods).
e) the last pieces which are mechanical: LLVMGetAtomicRMWBinOp and LLVMSetAtomicRMWBinOp once you know the mappings are correct, LLVMGetWeak and LLVMSetWeak not to be confused with linkage this sets or clears the weak bit on the cmpxchg instruction, ensure no other instruction has one.
f) the test is llvm-c-test/echo.cpp where we simply add the fence instruction and more setters and getters so that it clones volatile, weak, atomic order, etc.

arsenm added a subscriber: arsenm.Sep 10 2019, 12:09 PM

Tests?

llvm/lib/IR/Core.cpp
3643–3645 ↗(On Diff #219164)

Unrelated problem, but this won't catch memory intrinsics with volatile set. I also think there's another copy of this somewhere

nickwasmer marked an inline comment as done.

Added tests.

nickwasmer marked an inline comment as done.Sep 18 2019, 5:22 PM
nickwasmer added inline comments.
llvm/lib/IR/Core.cpp
3643–3645 ↗(On Diff #219164)

Unrelated problem, but this won't catch memory intrinsics with volatile set. I also think there's another copy of this somewhere

I looked but didn't spot another copy of this logic. If you could point me at it, I'd be happy to use it.

I'm ambivalent about memory intrinsics. The call instruction part of a memory intrinsic doesn't have a concept of volatility.

arsenm accepted this revision.Sep 25 2019, 2:19 PM

LGTM with nit

llvm/lib/IR/Core.cpp
3658–3659 ↗(On Diff #220585)

No else after return

This revision is now accepted and ready to land.Sep 25 2019, 2:19 PM
This revision was automatically updated to reflect the committed changes.