-
Notifications
You must be signed in to change notification settings - Fork 11.4k
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.
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
[C API] Fix LLVMGetOrdering/LLVMIsAtomicSingleThread for fence/memory instrs #65228
[C API] Fix LLVMGetOrdering/LLVMIsAtomicSingleThread for fence/memory instrs #65228
Conversation
From the build logs:
Possibly Windows Defender triggering on it? |
… instrs LLVMGetOrdering previously did not support Fence instructions, and calling it on a fence would lead to a bad cast as it assumed a load/store, or an AtomicRMWInst. This would either read a garbage memory order, or assertion LLVMIsAtomicSingleThread did not support either Fence instructions, loads, or stores, and would similarly lead to a bad cast These cases are now fixed for the C API, and tests for these instructions are added. The echo test utility now also supports cloning Fence instructions, which it did not previously
df4d141
to
b20e1a3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This all looks good to me (and I'm happy to see this change in order to resolve cdisselkoen/llvm-ir#44 and cdisselkoen/llvm-ir#15)
Bumping this in case it was missed: if anyone with merge access would be relevant to review or merge, I can ping them. I tried locally merging this into latest main and the tests still passed, so it should still be good in this state |
@@ -3769,6 +3771,8 @@ void LLVMSetOrdering(LLVMValueRef MemAccessInst, LLVMAtomicOrdering Ordering) { | |||
|
|||
if (LoadInst *LI = dyn_cast<LoadInst>(P)) | |||
return LI->setOrdering(O); | |||
else if (FenceInst *FI = dyn_cast<FenceInst>(P)) | |||
return FI->setOrdering(O); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing atomicrmw handling?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can add this as well. My understanding is that the bindings tests will not use that, since it uses LLVMBuildAtomicRMW
to set the ordering directly. Is that okay, or is there a way to also add a test for it?
@llvm/pr-subscribers-llvm-ir ChangesFixes #65227 LLVMGetOrdering previously did not support Fence instructions, and calling it on a fence would lead to a bad cast as it LLVMIsAtomicSingleThread did not support either Fence instructions, loads, or stores, and would similarly lead to a bad cast. These cases are now fixed for the C API, and tests for these instructions are added. The echo test utility now also supports cloning Fence instructions, which it did not previously From what I can tell, there's no unified API to pull Full diff: https://github.com/llvm/llvm-project/pull/65228.diff 3 Files Affected:
diff --git a/llvm/lib/IR/Core.cpp b/llvm/lib/IR/Core.cpp
index 17093fa0ac4ee1e..04d044b8d51e11b 100644
--- a/llvm/lib/IR/Core.cpp
+++ b/llvm/lib/IR/Core.cpp
@@ -3758,6 +3758,8 @@ LLVMAtomicOrdering LLVMGetOrdering(LLVMValueRef MemAccessInst) {
O = LI->getOrdering();
else if (StoreInst *SI = dyn_cast<StoreInst>(P))
O = SI->getOrdering();
+ else if (FenceInst *FI = dyn_cast<FenceInst>(P))
+ O = FI->getOrdering();
else
O = cast<AtomicRMWInst>(P)->getOrdering();
return mapToLLVMOrdering(O);
@@ -3769,6 +3771,10 @@ void LLVMSetOrdering(LLVMValueRef MemAccessInst, LLVMAtomicOrdering Ordering) {
if (LoadInst *LI = dyn_cast<LoadInst>(P))
return LI->setOrdering(O);
+ else if (FenceInst *FI = dyn_cast<FenceInst>(P))
+ return FI->setOrdering(O);
+ else if (AtomicRMWInst *ARWI = dyn_cast<AtomicRMWInst>(P))
+ return ARWI->setOrdering(O);
return cast<StoreInst>(P)->setOrdering(O);
}
@@ -4039,6 +4045,12 @@ LLVMBool LLVMIsAtomicSingleThread(LLVMValueRef AtomicInst) {
if (AtomicRMWInst *I = dyn_cast<AtomicRMWInst>(P))
return I->getSyncScopeID() == SyncScope::SingleThread;
+ else if (FenceInst *FI = dyn_cast<FenceInst>(P))
+ return FI->getSyncScopeID() == SyncScope::SingleThread;
+ else if (StoreInst *SI = dyn_cast<StoreInst>(P))
+ return SI->getSyncScopeID() == SyncScope::SingleThread;
+ else if (LoadInst *LI = dyn_cast<LoadInst>(P))
+ return LI->getSyncScopeID() == SyncScope::SingleThread;
return cast<AtomicCmpXchgInst>(P)->getSyncScopeID() ==
SyncScope::SingleThread;
}
@@ -4049,6 +4061,12 @@ void LLVMSetAtomicSingleThread(LLVMValueRef AtomicInst, LLVMBool NewValue) {
if (AtomicRMWInst *I = dyn_cast<AtomicRMWInst>(P))
return I->setSyncScopeID(SSID);
+ else if (FenceInst *FI = dyn_cast<FenceInst>(P))
+ return FI->setSyncScopeID(SSID);
+ else if (StoreInst *SI = dyn_cast<StoreInst>(P))
+ return SI->setSyncScopeID(SSID);
+ else if (LoadInst *LI = dyn_cast<LoadInst>(P))
+ return LI->setSyncScopeID(SSID);
return cast<AtomicCmpXchgInst>(P)->setSyncScopeID(SSID);
}
diff --git a/llvm/test/Bindings/llvm-c/atomics.ll b/llvm/test/Bindings/llvm-c/atomics.ll
index ba29e5e1e170862..e64a29944ef9df9 100644
--- a/llvm/test/Bindings/llvm-c/atomics.ll
+++ b/llvm/test/Bindings/llvm-c/atomics.ll
@@ -2,6 +2,40 @@
; RUN: llvm-as < %s | llvm-c-test --echo > %t.echo
; RUN: diff -w %t.orig %t.echo
+
+define void @fence_instrs() {
+ fence acquire
+ fence release
+ fence acq_rel
+ fence seq_cst
+
+ fence syncscope("singlethread") acquire
+ fence syncscope("singlethread") release
+ fence syncscope("singlethread") acq_rel
+ fence syncscope("singlethread") seq_cst
+
+ ret void
+}
+
+define void @atomic_load_store(ptr %word) {
+ ; Test different atomic loads
+ %ld.1 = load atomic i32, ptr %word monotonic, align 4
+ %ld.2 = load atomic volatile i32, ptr %word acquire, align 4
+ %ld.3 = load atomic volatile i32, ptr %word seq_cst, align 4
+ %ld.4 = load atomic volatile i32, ptr %word syncscope("singlethread") acquire, align 4
+ %ld.5 = load atomic volatile i32, ptr %word syncscope("singlethread") seq_cst, align 4
+ %ld.6 = load atomic i32, ptr %word syncscope("singlethread") seq_cst, align 4
+
+ ; Test different atomic stores
+ store atomic i32 1, ptr %word monotonic, align 4
+ store atomic volatile i32 2, ptr %word release, align 4
+ store atomic volatile i32 3, ptr %word seq_cst, align 4
+ store atomic volatile i32 4, ptr %word syncscope("singlethread") release, align 4
+ store atomic volatile i32 5, ptr %word syncscope("singlethread") seq_cst, align 4
+ store atomic i32 6, ptr %word syncscope("singlethread") seq_cst, align 4
+ ret void
+}
+
define i32 @main() {
%1 = alloca i32, align 4
%2 = cmpxchg ptr %1, i32 2, i32 3 seq_cst acquire
diff --git a/llvm/tools/llvm-c-test/echo.cpp b/llvm/tools/llvm-c-test/echo.cpp
index faf9838a0069afc..06966ce528eae4d 100644
--- a/llvm/tools/llvm-c-test/echo.cpp
+++ b/llvm/tools/llvm-c-test/echo.cpp
@@ -677,6 +677,7 @@ struct FunCloner {
LLVMSetAlignment(Dst, LLVMGetAlignment(Src));
LLVMSetOrdering(Dst, LLVMGetOrdering(Src));
LLVMSetVolatile(Dst, LLVMGetVolatile(Src));
+ LLVMSetAtomicSingleThread(Dst, LLVMIsAtomicSingleThread(Src));
break;
}
case LLVMStore: {
@@ -686,6 +687,7 @@ struct FunCloner {
LLVMSetAlignment(Dst, LLVMGetAlignment(Src));
LLVMSetOrdering(Dst, LLVMGetOrdering(Src));
LLVMSetVolatile(Dst, LLVMGetVolatile(Src));
+ LLVMSetAtomicSingleThread(Dst, LLVMIsAtomicSingleThread(Src));
break;
}
case LLVMGetElementPtr: {
@@ -891,6 +893,12 @@ struct FunCloner {
Dst = LLVMBuildFreeze(Builder, Arg, Name);
break;
}
+ case LLVMFence: {
+ LLVMAtomicOrdering Ordering = LLVMGetOrdering(Src);
+ LLVMBool IsSingleThreaded = LLVMIsAtomicSingleThread(Src);
+ Dst = LLVMBuildFence(Builder, Ordering, IsSingleThreaded, Name);
+ break;
+ }
default:
break;
}
|
Should I merge again from main, or is this good to land? |
Should be good to land. |
Is that something you can do, or should I reach out to someone else? I don't have access rights to merge it |
Oh, I thought you have commit access because you are marked as "Member". Apparently that also includes people in bz-contributors that do not have commit access as well. |
Local branch amd-gfx 89049b0 Merged main:dc1dc60ea878 into amd-gfx:b14bde5d0245 Remote branch main d222c5e [C API] Fix LLVMGetOrdering/LLVMIsAtomicSingleThread for fence/memory instrs (llvm#65228)
Fixes #65227
LLVMGetOrdering previously did not support Fence instructions, and calling it on a fence would lead to a bad cast as it
assumed a load/store, or an AtomicRMWInst. This would either read a garbage memory order, or assertion
LLVMIsAtomicSingleThread did not support either Fence instructions, loads, or stores, and would similarly lead to a bad cast.
It happened to work out since the relevant types all have their synch scope ID at the same offset, but it still should be fixed
These cases are now fixed for the C API, and tests for these instructions are added. The echo test utility now also supports cloning Fence instructions, which it did not previously
From what I can tell, there's no unified API to pull
getOrdering
/getSyncScopeID
from, and instead requires casting to individual types: if there is a better way of handling this I can switch to that