[LLVM] convergence verifier should visit all instructions

[ssahasra]

The entry and loop intrinsics for convergence control cannot be preceded by convergent operations in their respective basic blocks. To check that, the verifier needs to reset its state at the start of the block. This was missed in the previous commit fa6dd7a.

[llvmbot]

@llvm/pr-subscribers-llvm-adt

@llvm/pr-subscribers-llvm-ir

Changes

The entry and loop intrinsics for convergence control cannot be preceded by convergent operations in their respective basic blocks. To check that, the verifier needs to reset its state at the start of the block. This was missed in the previous commit fa6dd7a. -- Full diff: https://github.com//pull/66200.diff

2 Files Affected:

• (modified) llvm/lib/IR/Verifier.cpp (+2-2)
• (modified) llvm/test/Verifier/convergencectrl-invalid.ll (+6-1)

diff --git a/llvm/lib/IR/Verifier.cpp b/llvm/lib/IR/Verifier.cpp
index c0f30a62b8bccc3..9ed66e6af68fbfc 100644
--- a/llvm/lib/IR/Verifier.cpp
+++ b/llvm/lib/IR/Verifier.cpp
@@ -3576,8 +3576,6 @@ void Verifier::visitCallBase(CallBase &Call) {
   if (Call.isInlineAsm())
     verifyInlineAsmCall(Call);
 
-  CV.visit(Call);
-
   visitInstruction(Call);
 }
 
@@ -4805,6 +4803,8 @@ void Verifier::visitInstruction(Instruction &I) {
   BasicBlock *BB = I.getParent();
   Check(BB, "Instruction not embedded in basic block!", &I);
 
+  CV.visit(I);
+
   if (!isa<PHINode>(I)) {   // Check that non-phi nodes are not self referential
     for (User *U : I.users()) {
       Check(U != (User *)&I || !DT.isReachableFromEntry(BB),
diff --git a/llvm/test/Verifier/convergencectrl-invalid.ll b/llvm/test/Verifier/convergencectrl-invalid.ll
index 2f7b311973d7e1e..e1fffcd1c603347 100644
--- a/llvm/test/Verifier/convergencectrl-invalid.ll
+++ b/llvm/test/Verifier/convergencectrl-invalid.ll
@@ -126,13 +126,18 @@ define void @entry_in_convergent(i32 %x, i32 %y) {
 }
 
 ; CHECK: Loop intrinsic cannot be preceded by a convergent operation in the same basic block.
-; CHECK:   %t60_tok3
+; CHECK-NEXT: %h1
+; CHECK-SAME: %t60_tok3
 define void @loop_at_start(i32 %x, i32 %y) convergent {
 A:
   %t60_tok3 = call token @llvm.experimental.convergence.entry()
   br label %B
 B:
   %z = add i32 %x, %y
+  ; This is not an error
+  %h2 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %t60_tok3) ]
+  br label %C
+C:
   call void @f()
   %h1 = call token @llvm.experimental.convergence.loop() [ "convergencectrl"(token %t60_tok3) ]
   ret void

[arsenm]

CV should probably be renamed, I had no idea what this was

[ssahasra]

Fixed that.

[arsenm]

If only calls can be convergent, I'm not sure I see how this is different. I don't really expect a visit instruction for the verifier to be side effecting. Can you have a separate enter block visitor?

[ruiling]

The ConvergenceVerifier may also keep record of the last-call-instruction's parent. Then it is easy to detect entering a new block inside ConvergenceVerifier::visit(). I have no opinion which is the best way to way.

[ssahasra]

Fixed this by adding a visitBlock() method to the convergence verifier.

[nhaehnle]

Maybe make that explicit with a CHECK-NOT: %h2?

[ssahasra]

That's what the "CHECK-NEXT: %h1" does. It ensures that the error message is for %h1 and not %h2. There is not enough context in the stream of error messages to put a CHECK-NOT here.

[ssahasra]

Bump! Waiting for this one to the next PR that introduces convergence control operations and LLVM::token() type in GlobalISel.

Will be restarting this review as a new PR:
https://reviews.llvm.org/D158147

[jayfoad]

LGTM