-
Notifications
You must be signed in to change notification settings - Fork 14.3k
[AMDGPU] Enhance verification of amdgcn.cs.chain intrinsic #128162
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
Conversation
Make sure that this intrinsic is being followed by unreachable. This LLVM defect was identified via the AMD Fuzzing project.
@llvm/pr-subscribers-llvm-ir Author: Robert Imschweiler (ro-i) ChangesMake sure that this intrinsic is being followed by unreachable. This LLVM defect was identified via the AMD Fuzzing project. Thanks to @rovka for helping me solve this issue! Full diff: https://github.com/llvm/llvm-project/pull/128162.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 8432779c107de..0ba3c345795b7 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6367,6 +6367,8 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
"SGPR arguments must have the `inreg` attribute", &Call);
Check(!Call.paramHasAttr(3, Attribute::InReg),
"VGPR arguments must not have the `inreg` attribute", &Call);
+ Check(dyn_cast_or_null<UnreachableInst>(Call.getNextNode()),
+ "amdgcn_cs_chain must precede unreachable", &Call);
break;
}
case Intrinsic::amdgcn_set_inactive_chain_arg: {
diff --git a/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll b/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll
index b9e6e1eb45905..cd66e6fdf4ceb 100644
--- a/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll
+++ b/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll
@@ -32,6 +32,13 @@ define amdgpu_cs_chain void @bad_exec(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr,
unreachable
}
+define amdgpu_cs_chain void @not_unreachable(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr, { ptr, <3 x i32> } %vgpr) {
+ ; CHECK: amdgcn_cs_chain must precede unreachable
+ ; CHECK-NEXT: @llvm.amdgcn.cs.chain
+ call void(ptr, i32, <4 x i32>, { ptr, <3 x i32> }, i32, ...) @llvm.amdgcn.cs.chain(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr, { ptr, <3 x i32> } %vgpr, i32 0)
+ ret void
+}
+
define void @bad_caller_default_cc(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr, { ptr, <3 x i32> } %vgpr) {
; CHECK: Intrinsic can only be used from functions with the amdgpu_cs_chain or amdgpu_cs_chain_preserve calling conventions
; CHECK-NEXT: @llvm.amdgcn.set.inactive.chain.arg
@@ -117,4 +124,4 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_inreg(ptr addrspace(1) %out,
%tmp = call i32 @llvm.amdgcn.set.inactive.chain.arg(i32 %active, i32 inreg %inactive) #0
store i32 %tmp, ptr addrspace(1) %out
ret void
-}
+}
\ No newline at end of file
|
@llvm/pr-subscribers-backend-amdgpu Author: Robert Imschweiler (ro-i) ChangesMake sure that this intrinsic is being followed by unreachable. This LLVM defect was identified via the AMD Fuzzing project. Thanks to @rovka for helping me solve this issue! Full diff: https://github.com/llvm/llvm-project/pull/128162.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 8432779c107de..0ba3c345795b7 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -6367,6 +6367,8 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) {
"SGPR arguments must have the `inreg` attribute", &Call);
Check(!Call.paramHasAttr(3, Attribute::InReg),
"VGPR arguments must not have the `inreg` attribute", &Call);
+ Check(dyn_cast_or_null<UnreachableInst>(Call.getNextNode()),
+ "amdgcn_cs_chain must precede unreachable", &Call);
break;
}
case Intrinsic::amdgcn_set_inactive_chain_arg: {
diff --git a/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll b/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll
index b9e6e1eb45905..cd66e6fdf4ceb 100644
--- a/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll
+++ b/llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll
@@ -32,6 +32,13 @@ define amdgpu_cs_chain void @bad_exec(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr,
unreachable
}
+define amdgpu_cs_chain void @not_unreachable(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr, { ptr, <3 x i32> } %vgpr) {
+ ; CHECK: amdgcn_cs_chain must precede unreachable
+ ; CHECK-NEXT: @llvm.amdgcn.cs.chain
+ call void(ptr, i32, <4 x i32>, { ptr, <3 x i32> }, i32, ...) @llvm.amdgcn.cs.chain(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr, { ptr, <3 x i32> } %vgpr, i32 0)
+ ret void
+}
+
define void @bad_caller_default_cc(ptr %fn, i32 %exec, <4 x i32> inreg %sgpr, { ptr, <3 x i32> } %vgpr) {
; CHECK: Intrinsic can only be used from functions with the amdgpu_cs_chain or amdgpu_cs_chain_preserve calling conventions
; CHECK-NEXT: @llvm.amdgcn.set.inactive.chain.arg
@@ -117,4 +124,4 @@ define amdgpu_cs_chain void @set_inactive_chain_arg_inreg(ptr addrspace(1) %out,
%tmp = call i32 @llvm.amdgcn.set.inactive.chain.arg(i32 %active, i32 inreg %inactive) #0
store i32 %tmp, ptr addrspace(1) %out
ret void
-}
+}
\ No newline at end of file
|
llvm/lib/IR/Verifier.cpp
Outdated
@@ -6367,6 +6367,8 @@ void Verifier::visitIntrinsicCall(Intrinsic::ID ID, CallBase &Call) { | |||
"SGPR arguments must have the `inreg` attribute", &Call); | |||
Check(!Call.paramHasAttr(3, Attribute::InReg), | |||
"VGPR arguments must not have the `inreg` attribute", &Call); | |||
Check(dyn_cast_or_null<UnreachableInst>(Call.getNextNode()), |
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.
isa_or_null?
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 changed it to isa_and_present. Is that what you meant?
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.
Looks good, thanks! Please address Matt's comment before you merge.
This intrinsic should really be required to use callbr. There isn't a sensible way for generic IR transforms to respect this condition, it just happens it's unlikely something would want to place instructions at the end of a block ends in unreachable |
Hm, but in that case, we would first need to implement |
Right, we would need to fix the definition of callbr to permit intrinsics instead of only asm. We have several intrinsics which should naturally be terminators, but are not simply due to predating callbr and callbr starting out limited to asm |
Make sure that this intrinsic is being followed by unreachable.
This LLVM defect was identified via the AMD Fuzzing project.
Thanks to @rovka for helping me solve this issue!