Skip to content

[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

Merged
merged 3 commits into from
Feb 21, 2025

Conversation

ro-i
Copy link
Contributor

@ro-i ro-i commented Feb 21, 2025

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.
@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2025

@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<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

@llvmbot
Copy link
Member

llvmbot commented Feb 21, 2025

@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<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

@@ -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()),
Copy link
Contributor

Choose a reason for hiding this comment

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

isa_or_null?

Copy link
Contributor Author

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?

Copy link
Collaborator

@rovka rovka left a 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.

@arsenm arsenm merged commit fb25216 into llvm:main Feb 21, 2025
11 checks passed
@ro-i ro-i deleted the verify-cs-chain branch February 21, 2025 20:09
@arsenm
Copy link
Contributor

arsenm commented Feb 23, 2025

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

@ro-i
Copy link
Contributor Author

ro-i commented Feb 23, 2025

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.

@arsenm
Copy link
Contributor

arsenm commented Feb 23, 2025

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants