SimplifyCFG: SinkThenElseCode shouldn't sink convergent functions

[arsenm]

I think this is a legal transformation. For example if you have a barrier like this with a divergent condition, the source program is broken. You can infer from this that %cond is a uniform condition

[jlebar]

In D30921#700043, @arsenm wrote:

I think this is a legal transformation. For example if you have a barrier like this with a divergent condition, the source program is broken. You can infer from this that %cond is a uniform condition

I don't think it's correct to infer that if you call a convergent function from within a branch that the branch is uniform. For one thing, the function may not actually call a barrier function -- "convergent" is a *restriction* on the optimizer, but it's not a promise to the optimizer that a function does anything in particular. For example, in CUDA code clang marks all functions as convergent, and then relies on LLVM to remove the attr where it can prove that the function is not convergent. But this analysis is not complete (I can provide examples if this is needed, but one obvious case is, all inline asm is opaque and assumed to be convergent).

Nonetheless I agree with Matt that this optimization is correct, just for a different reason. As spec'ed in the langref, you cannot *add* control-flow dependencies to calls to convergent functions. You are allowed to *remove* dependencies, however. This transformation does exactly that.

lib/Transforms/Utils/SimplifyCFG.cpp

for (auto *I : Insts) {
     if (isa<PHINode>(I) || I->isEHPad() || isa<AllocaInst>(I) ||
         I->getType()->isTokenTy())
       return false;
 
     // Conservatively return false if I is an inline-asm instruction. Sinking
     // and merging inline-asm instructions can potentially create arguments
     // that cannot satisfy the inline-asm constraints.
     if (const auto *C = dyn_cast<CallInst>(I))
-      if (C->isInlineAsm())
+      if (C->isInlineAsm() || C->isConvergent())
         return false;
 
     // Everything must have only one use too, apart from stores which
     // have no uses.
     if (!isa<StoreInst>(I) && !I->hasOneUse())
       return false;
   }
 

test/Transforms/SimplifyCFG/sink-common-code.ll

 }
 
 ; CHECK-LABEL: @test_operand_bundles(
 ; CHECK: left:
 ; CHECK-NEXT:   %val0 = call i32 @call_target() [ "deopt"(i32 10) ]
 ; CHECK: right:
 ; CHECK-NEXT:   %val1 = call i32 @call_target() [ "deopt"(i32 20) ]
 
+declare i32 @convergent_call(i32)
+
+define i32 @test_no_sink_convergent(i1 %cond) {
+entry:
+  br i1 %cond, label %if.then, label %if.else
+
+if.then:
+  %tmp0 = call i32 @convergent_call(i32 0) #0
+  br label %if.end
+
+if.else:
+  %tmp1 = call i32 @convergent_call(i32 1) #0
+  br label %if.end
+
+if.end:
+  %tmp2 = phi i32 [%tmp0, %if.then], [%tmp1, %if.else]
+  ret i32 %tmp2
+}
+
+; CHECK-LABEL: @convergent_call
+; CHECK: @convergent_call(i32 0)
+; CHECK: @convergent_call(i32 1)
+
+attributes #0 = { convergent }
+
 ; CHECK: ![[TBAA]] = !{![[TYPE:[0-9]]], ![[TYPE]], i64 0}
 ; CHECK: ![[TYPE]] = !{!"float", ![[TEXT:[0-9]]]}
 ; CHECK: ![[TEXT]] = !{!"an example type tree"}