[AMDGPU] Enhance verification of amdgcn.cs.chain intrinsic by ro-i · Pull Request #128162 · llvm/llvm-project (original) (raw)
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 andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Conversation11 Commits3 Checks12 Files changed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.Learn more about bidirectional Unicode characters
[ Show hidden characters]({{ revealButtonHref }})
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!
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)
Changes
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!
Full diff: https://github.com/llvm/llvm-project/pull/128162.diff
2 Files Affected:
- (modified) llvm/lib/IR/Verifier.cpp (+2)
- (modified) llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll (+8-1)
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(Call.getNextNode()),
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 }"amdgcn_cs_chain must precede unreachable", &Call);
+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)
Changes
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!
Full diff: https://github.com/llvm/llvm-project/pull/128162.diff
2 Files Affected:
- (modified) llvm/lib/IR/Verifier.cpp (+2)
- (modified) llvm/test/Verifier/AMDGPU/intrinsic-amdgpu-cs-chain.ll (+8-1)
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(Call.getNextNode()),
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 }"amdgcn_cs_chain must precede unreachable", &Call);
+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
| @@ -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(Call.getNextNode()), |
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isa_or_null?
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?
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 callbr ^^
Because for GlobalISel, it isn't implemented at all, and for SelDAG, the code and the docs say that it's (currently) only "to implement the 'goto' feature of gcc inline assembly". ("This instruction should only be used to implement the “goto” feature of gcc style inline assembly. Any other usage is an error in the IR verifier.")
Why do you think that callbr would be a better option than e.g. enforcing tail call and stopping to emit?
See also the GCC docs on the goto with labels.
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