Skip to content

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

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
+13 −1
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
66070f4
IR/Verifier: Do not allow kernel to kernel calls.
jofrn Jun 16, 2025
1f46318
Merge rules together.
jofrn Jun 17, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion llvm/lib/IR/Verifier.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3632,8 +3632,11 @@ void Verifier::visitCallBase(CallBase &Call) {
Check(Callee->getValueType() == FTy,
"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();
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

@jofrn jofrn Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

// Verify if the calling convention of the callee is callable.
Check(isCallableCC(Call.getCallingConv()),
Check(isCallableCC(CalleeCC),
"calling convention does not permit calls", Call);

// Disallow passing/returning values with alignment higher than we can
Expand Down
9 changes: 9 additions & 0 deletions llvm/test/Verifier/AMDGPU/kernel-recursivecall.ll
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
; RUN: not llvm-as %s -disable-output 2>&1 | FileCheck %s

define amdgpu_kernel void @kernel(ptr addrspace(1) %out, i32 %n) {
entry:
; CHECK: calling convention does not permit calls
; CHECK-NEXT: call void @kernel(ptr addrspace(1) %out, i32 %n)
call void @kernel(ptr addrspace(1) %out, i32 %n)
Copy link
Contributor

@shiltian shiltian Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@jofrn jofrn Jun 17, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

ret void
}
Loading