Skip to content

[SPIRV] convergence anchor intrinsic does not have a parent token #122230

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

Merged
merged 2 commits into from
Jan 13, 2025
Merged

[SPIRV] convergence anchor intrinsic does not have a parent token #122230

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
f474d32
[SPIRV] convergence anchor intrinsic does not have a parent token
ssahasra Jan 9, 2025
74bd77b
add asserts to catch tokens incorrectly placed on entry or anchor
ssahasra Jan 10, 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
6 changes: 3 additions & 3 deletions llvm/include/llvm/IR/IntrinsicInst.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1873,13 +1873,13 @@ class ConvergenceControlInst : public IntrinsicInst {
return isa<IntrinsicInst>(V) && classof(cast<IntrinsicInst>(V));
}

bool isAnchor() {
bool isAnchor() const {
return getIntrinsicID() == Intrinsic::experimental_convergence_anchor;
}
bool isEntry() {
bool isEntry() const {
return getIntrinsicID() == Intrinsic::experimental_convergence_entry;
}
bool isLoop() {
bool isLoop() const {
return getIntrinsicID() == Intrinsic::experimental_convergence_loop;
}
};
Expand Down
20 changes: 6 additions & 14 deletions llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,20 +56,12 @@ getConvergenceTokenInternal(BasicBlockType *BB) {
"Output type must be an intrinsic instruction.");

for (auto &I : *BB) {
if (auto *II = dyn_cast<IntrinsicInst>(&I)) {
switch (II->getIntrinsicID()) {
case Intrinsic::experimental_convergence_entry:
case Intrinsic::experimental_convergence_loop:
return II;
case Intrinsic::experimental_convergence_anchor: {
auto Bundle = II->getOperandBundle(LLVMContext::OB_convergencectrl);
assert(Bundle->Inputs.size() == 1 &&
Copy link
Collaborator Author

@ssahasra ssahasra Jan 10, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

This assert is wrong. The only valid assert here is to say assert(!Bundle), because the bundle cannot exist. At that point, the only thing that can be returned is II itself, because TII cannot exist.

Copy link
Contributor

@s-perron s-perron Jan 10, 2025

Choose a reason for hiding this comment

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

Agreed this assert is wrong, but the question I have is if the analysis will do the correct thing if II is returned. I'm guessing we do not test that.

Bundle->Inputs[0]->getType()->isTokenTy());
auto TII = dyn_cast<IntrinsicInst>(Bundle->Inputs[0].get());
assert(TII != nullptr);
return TII;
}
}
if (auto *CI = dyn_cast<ConvergenceControlInst>(&I)) {
// Make sure that the anchor or entry intrinsics did not reach here with a
// parent token. This should have failed the verifier.
assert(CI->isLoop() ||
!CI->getOperandBundle(LLVMContext::OB_convergencectrl));
return CI;
}

if (auto *CI = dyn_cast<CallInst>(&I)) {
Expand Down
Loading