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

[ssahasra]

No description provided.

[llvmbot]

@llvm/pr-subscribers-llvm-ir

@llvm/pr-subscribers-backend-spir-v

Author: Sameer Sahasrabuddhe (ssahasra)

Changes

---

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

1 Files Affected:

• (modified) llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp (+2-14)

diff --git a/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp b/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
index cc6daf7ef34426..9ac0f8700a3f66 100644
--- a/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
+++ b/llvm/lib/Target/SPIRV/Analysis/SPIRVConvergenceRegionAnalysis.cpp
@@ -56,20 +56,8 @@ 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 &&
-               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)) {
+      return CI;
     }
 
     if (auto *CI = dyn_cast<CallInst>(&I)) {

[s-perron]

Do you have a test case? I'd like to see how this effects the generation of the merge instructions. Thanks.

[ssahasra]

Do you have a test case? I'd like to see how this effects the generation of the merge instructions. Thanks.

The way I see it, the request should be turned around. Is there an existing test case that even reaches here? This code is clearly wrong and cannot possibly do anything. Any test for it will be rejected by the verifier.

https://llvm.org/docs/ConvergentOperations.html#llvm-experimental-convergence-anchor

It is an error to pass a convergencectrl operand bundle at a call to this intrinsic.

[s-perron]

Is there an existing test case that even reaches here?

That is a good question. I thought there would be. If you have a valid use of Intrinsic::experimental_convergence_anchor, then it would execute that case in the old code, and would assert. That would be a test case that executes the old code, and shows the need for it to be fixed.

Your change would avoid the assert, and do something. However, I'm not convinced that what it is doing is the right thing.

Sorry for my ignorance, but I'm not sure what a reasonable and expected us of the anchor is. That is why I cannot reason about it very well. I'm assuming the existing test do not use the anchor intrinsic because it would have either failed the verifier (as you said) or asserted.

[ssahasra]

I really don't understand what assert you are talking about. The code that I deleted tries to return a token used by an anchor, which cannot exist because a legal anchor should not have an operand bundle. I don't know how to be clearer than this. That code is wrong, and it should be removed.

[ssahasra]

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.

[s-perron]

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.

[ssahasra]

As a concession to existing SPIRV implementation, added an assert that catches any incorrectly placed tokens on the anchor and entry intrinsics at this point. Anything that trips this assert is a bug in the use of these intrinsics and needs to be fixed separately. I don't think anything more is possible in this patch.

[Keenuts]

Thanks! So far none of the tests/code we have use an anchor, and not sure what's a valid use case for those yet. But this patch looks good, thanks for fixing!

[s-perron]

We look into what to do if there is an anchor in the llvm-ir. This is fine for now.