-
Notifications
You must be signed in to change notification settings - Fork 15.2k
[Verifier][AMDGPU] Restrict targets for chain calling convention. #157469
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
base: main
Are you sure you want to change the base?
Conversation
…lar situations like being on an architecture it cannot compile to.
@llvm/pr-subscribers-backend-amdgpu @llvm/pr-subscribers-llvm-ir Author: None (jofrn) ChangesThe chain calling convention is invalid in particular situations like being on an architecture it cannot compile to. Full diff: https://github.com/llvm/llvm-project/pull/157469.diff 2 Files Affected:
diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 1d3c379f461fa..63ae083fec8e8 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2938,10 +2938,16 @@ void Verifier::visitFunction(const Function &F) {
"Calling convention parameter requires byval", &F);
break;
}
- case CallingConv::AMDGPU_KERNEL:
- case CallingConv::SPIR_KERNEL:
case CallingConv::AMDGPU_CS_Chain:
case CallingConv::AMDGPU_CS_ChainPreserve:
+ {
+ auto TT = M.getTargetTriple().str();
+ if (TT.find("gfx1200") || TT.find("gfx942"))
+ Check(false, "Chain calling convention is invalid on this target", &F);
+ }
+ [[fallthrough]];
+ case CallingConv::AMDGPU_KERNEL:
+ case CallingConv::SPIR_KERNEL:
Check(F.getReturnType()->isVoidTy(),
"Calling convention requires void return type", &F);
[[fallthrough]];
diff --git a/llvm/test/CodeGen/AMDGPU/amdgpu-cs-chain-invalid-arch.ll b/llvm/test/CodeGen/AMDGPU/amdgpu-cs-chain-invalid-arch.ll
new file mode 100644
index 0000000000000..c6f8a09eaf5b6
--- /dev/null
+++ b/llvm/test/CodeGen/AMDGPU/amdgpu-cs-chain-invalid-arch.ll
@@ -0,0 +1,6 @@
+; RUN: not llc -mtriple=amdgcn -mcpu=gfx1200 -o - < %s 2>&1 | FileCheck %s
+
+define amdgpu_cs_chain void @test_alloca() {
+; CHECK: Chain calling convention is invalid on this target
+ ret void
+}
|
You can test this locally with the following command:git-clang-format --diff origin/main HEAD --extensions cpp -- llvm/lib/IR/Verifier.cpp
View the diff from clang-format here.diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 4f2aa4005..bf05113c8 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -2933,8 +2933,7 @@ void Verifier::visitFunction(const Function &F) {
break;
}
case CallingConv::AMDGPU_CS_Chain:
- case CallingConv::AMDGPU_CS_ChainPreserve:
- {
+ case CallingConv::AMDGPU_CS_ChainPreserve: {
auto TT = M.getTargetTriple().str();
if (TT.find("gfx1200") || TT.find("gfx942"))
Check(false, "Chain calling convention is invalid on this target", &F);
|
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.
You cannot rely on this subtarget check
; RUN: not llc -mtriple=amdgcn -mcpu=gfx1200 -o - < %s 2>&1 | FileCheck %s | ||
|
||
define amdgpu_cs_chain void @test_alloca() { | ||
; CHECK: Chain calling convention is invalid on this target |
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 would just fix this restriction, this seems totally artificial
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.
#132711 fixes the restriction, but they mentioned that the IR is invalid for this architecture, so the PR was never approved.
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 reject the notion that IR can be invalid for an architecture
case CallingConv::AMDGPU_CS_ChainPreserve: | ||
{ | ||
auto TT = M.getTargetTriple().str(); | ||
if (TT.find("gfx1200") || TT.find("gfx942")) |
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 is unacceptable. We should never do this.
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.
What other way does one check for subtarget?
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.
The IR verifier cannot and should not check for the subtarget. We cannot have subtarget dependent IR valid rules
The chain calling convention is invalid in particular situations like being on an architecture it cannot compile to.