IR/Verifier: Do not allow kernel to kernel calls.

[jofrn]

Allowing a kernel to call another kernel is invalid. This extends the Verifier to recognize this fact. The calling convention is retrieved from the module because it may not be labeling the callsite.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-amdgpu

Author: None (jofrn)

Changes

Allowing a kernel to call another kernel is invalid. This extends the Verifier to recognize this fact. The calling convention is retrieved from the module because it may not be labeling the callsite.

---

Full diff: https://github.com/llvm/llvm-project/pull/144445.diff

2 Files Affected:

• (modified) llvm/lib/IR/Verifier.cpp (+12)
• (added) llvm/test/Verifier/AMDGPU/kernel-recursivecall.ll (+9)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index 592bb6aa90613..179cb85ca04f4 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -3636,6 +3636,18 @@ void Verifier::visitCallBase(CallBase &Call) {
   Check(isCallableCC(Call.getCallingConv()),
         "calling convention does not permit calls", Call);
 
+  // Find the actual CC of the callee from the Module.
+  CallingConv::ID CalleeCC = Call.getParent()->getParent()->getParent()
+      ->getFunction(Call.getCalledFunction()->getName())->getCallingConv();
+  // Verify that a kernel does not call another kernel.
+  if (CalleeCC == CallingConv::AMDGPU_KERNEL ||
+      CalleeCC == CallingConv::SPIR_KERNEL) {
+    CallingConv::ID CallerCC = Call.getParent()->getParent()->getCallingConv();
+    Check(CallerCC != CallingConv::AMDGPU_KERNEL &&
+          CallerCC != CallingConv::SPIR_KERNEL,
+          "a kernel may not call a kernel", Call.getParent()->getParent());
+  }
+
   // Disallow passing/returning values with alignment higher than we can
   // represent.
   // FIXME: Consider making DataLayout cap the alignment, so this isn't
diff --git a/llvm/test/Verifier/AMDGPU/kernel-recursivecall.ll b/llvm/test/Verifier/AMDGPU/kernel-recursivecall.ll
new file mode 100755
index 0000000000000..83e8f6dadedfe
--- /dev/null
+++ b/llvm/test/Verifier/AMDGPU/kernel-recursivecall.ll
@@ -0,0 +1,9 @@
+; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s
+
+define amdgpu_kernel void @kernel(ptr addrspace(1) %out, i32 %n) {
+entry:
+; CHECK: a kernel may not call a kernel
+; CHECK-NEXT: ptr @kernel
+  call void @kernel(ptr addrspace(1) %out, i32 %n)
+  ret void
+}

[github-actions]

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:

git-clang-format --diff HEAD~1 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 69f05bf0b..2a1b39348 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -3622,11 +3622,15 @@ void Verifier::visitCallBase(CallBase &Call) {
           "Intrinsic called with incompatible signature", Call);
 
   // Find the actual CC of the callee from the Module.
-  CallingConv::ID CalleeCC = Call.getParent()->getParent()->getParent()
-      ->getFunction(Call.getCalledFunction()->getName())->getCallingConv();
+  CallingConv::ID CalleeCC =
+      Call.getParent()
+          ->getParent()
+          ->getParent()
+          ->getFunction(Call.getCalledFunction()->getName())
+          ->getCallingConv();
   // Verify if the calling convention of the callee is callable.
-  Check(isCallableCC(CalleeCC),
-        "calling convention does not permit calls", Call);
+  Check(isCallableCC(CalleeCC), "calling convention does not permit calls",
+        Call);
 
   // Disallow passing/returning values with alignment higher than we can
   // represent.

[shiltian]

duplicate to #134910?

[arsenm]

Duplicate?

[arsenm]

-disable-output is canonical way to disable output, also this does not require AMDGPU

[arsenm]

spir_kernel case not tested, nor call sites

[nikic]

Wat?

[nikic]

This is a very complicated way to spell Callee->getCallingConv() -- but note that Callee may be null here for indirect calls -- which is the cause of your four thousand or so test failures.

[jofrn]

Right, it is, but we can have a call of the function without its CC specified. If its definition has a kernel CC, then the callsite may be treated as requiring the definition when it does not have it. If the Verifier checks from the module, then we will not error out in a location that requires the callsite to have the CC at its definition; we will error in the Verifier instead.

[shiltian]

This IR is malformed in the first place because it doesn't use the right CC for the callee.

[jofrn]

It ends up getting the same error as your other one "Mark entry as invalid" if we do not have the CC match though, so it would be better to have this error in the Verifier.

[arsenm]

I think we should deal with this in codegen. From an IR perspective, nothing is structurally wrong. Codegen should still be able to set up a call to the kernel as-if it were a callable function

[shiltian]

I don't think it'd be a good idea. It will break a lot of assumptions and conventions we have for kernel function.

[arsenm]

No it doesn't. This is still a UB call that can be treated as a no-op

[arsenm]

Wouldn't this rely on Codegen retrieving the CC from the module as well?

Not sure what you mean by this, but no. The calling convention is always known at the callsite and is absolute. The fact that the call target happens to be uncallable doesn't matter for the purposes of the call. The resource analysis looking for call sites should just ignore this call, it isn't real. It's impossible and can be treated as unreachable code

[shiltian]

The calling convention is always known at the callsite and is absolute.

That's why I said this IR is ill-formed in the first place.

[arsenm]

The IR is not ill-formed, it exhibits statically knowable undefined behavior

[shiltian]

which is that the callee's CC doesn't match the call site's, and it should be a separate issue.

[arsenm]

Yes, there is no verifier issue to solve here. The verifier is working as intended now