Fix the assert in Annotate the loop in SIAnnotateControlFlow pass when the loop terminator condition is a constant.

[cfang]

Summary

If the condition is a constant, we insert a IfBreak intrinsic immediately before the Terminator.
This will fix the assert.

[arsenm]

The test is missing check lines, so FileCheck will error on it

test/CodeGen/AMDGPU/si-annotate-cfg-loop-assert.ll

1–2 ↗

(On Diff #41431)

Isn't this fixing the test? Why is this xfailed?

6–7 ↗

(On Diff #41431)

These should be removed

11 ↗

(On Diff #41431)

Remove the align 2

15 ↗

(On Diff #41431)

Can you run opt -strip -instnamer on this test?

[cfang]

Updated based on Matt's comments.

[cfang]

indeed update the LIT test based on Matt's comment.

[• tstellarAMD]

lib/Target/AMDGPU/SIAnnotateControlFlow.cpp

276–278 ↗

(On Diff #41447)

Usually getting a branch on constant in the SIAnnotateControlFlow means that there is a bug in the StructurizeCFG pass.
There is no harm in handling this here, but I think it may just be covering over a bug in the structurizer pass.

[cfang]

lib/Target/AMDGPU/SIAnnotateControlFlow.cpp

276–278 ↗

(On Diff #41447)

Why do you think there is a bug in the structurizer? My understanding is that, for an infinite loop or a one-trip (do-while) loop, the condition can be a constant, even though we may be able to optimize away the loop for the later case.

[cfang]

Add CHECKS

[• tstellarAMD]

lib/Target/AMDGPU/SIAnnotateControlFlow.cpp

276–278 ↗

(On Diff #41528)

This kind of failure is a common symptom of strurcturizer bugs. You should run llc with -print-before-all to see if the constant branch exists before the structurizer is run.

[cfang]

Any further comment?

lib/Target/AMDGPU/SIAnnotateControlFlow.cpp

276–278 ↗

(On Diff #41528)

It is true that the structrizer has a bug generating wrong constant condition. However, in a general sense, nothing prevents
the condition from being constant. And as a result, we should handle constant condition in annotating loops.

[arsenm]

LGTM

llvm/trunk/lib/Target/AMDGPU/SIAnnotateControlFlow.cpp

class SIAnnotateControlFlow : public FunctionPass {
   bool isElse(PHINode *Phi);
 
   void eraseIfUnused(PHINode *Phi);
 
   void openIf(BranchInst *Term);
 
   void insertElse(BranchInst *Term);
 
-  Value *handleLoopCondition(Value *Cond, PHINode *Broken, llvm::Loop *L);
+  Value *handleLoopCondition(Value *Cond, PHINode *Broken,
+                             llvm::Loop *L, BranchInst *Term);
 
   void handleLoop(BranchInst *Term);
 
   void closeControlFlow(BasicBlock *BB);
 
 public:
   static char ID;
 

 void SIAnnotateControlFlow::insertElse(BranchInst *Term) {
   Value *Ret = CallInst::Create(Else, popSaved(), "", Term);
   Term->setCondition(ExtractValueInst::Create(Ret, 0, "", Term));
   push(Term->getSuccessor(1), ExtractValueInst::Create(Ret, 1, "", Term));
 }
 
 /// \brief Recursively handle the condition leading to a loop
 Value *SIAnnotateControlFlow::handleLoopCondition(Value *Cond, PHINode *Broken,
-                                                  llvm::Loop *L) {
+                                             llvm::Loop *L, BranchInst *Term) {
 
   // Only search through PHI nodes which are inside the loop.  If we try this
   // with PHI nodes that are outside of the loop, we end up inserting new PHI
   // nodes outside of the loop which depend on values defined inside the loop.
   // This will break the module with
   // 'Instruction does not dominate all users!' errors.
   PHINode *Phi = nullptr;
   if ((Phi = dyn_cast<PHINode>(Cond)) && L->contains(Phi)) {
 
     BasicBlock *Parent = Phi->getParent();
     PHINode *NewPhi = PHINode::Create(Int64, 0, "", &Parent->front());
     Value *Ret = NewPhi;
 
     // Handle all non-constant incoming values first
     for (unsigned i = 0, e = Phi->getNumIncomingValues(); i != e; ++i) {
       Value *Incoming = Phi->getIncomingValue(i);
       BasicBlock *From = Phi->getIncomingBlock(i);
       if (isa<ConstantInt>(Incoming)) {
         NewPhi->addIncoming(Broken, From);
         continue;
       }
 
       Phi->setIncomingValue(i, BoolFalse);
-      Value *PhiArg = handleLoopCondition(Incoming, Broken, L);
+      Value *PhiArg = handleLoopCondition(Incoming, Broken, L, Term);
       NewPhi->addIncoming(PhiArg, From);
     }
 
     BasicBlock *IDom = DT->getNode(Parent)->getIDom()->getBlock();
 
     for (unsigned i = 0, e = Phi->getNumIncomingValues(); i != e; ++i) {
 
       Value *Incoming = Phi->getIncomingValue(i);
if ((Phi = dyn_cast<PHINode>(Cond)) && L->contains(Phi)) {
     if (L->contains(Inst)) {
       Insert = Parent->getTerminator();
     } else {
       Insert = L->getHeader()->getFirstNonPHIOrDbgOrLifetime();
     }
     Value *Args[] = { Cond, Broken };
     return CallInst::Create(IfBreak, Args, "", Insert);
 
+  // Insert IfBreak before TERM for constant COND.
+  } else if (isa<ConstantInt>(Cond)) {
+    Value *Args[] = { Cond, Broken };
+    return CallInst::Create(IfBreak, Args, "", Term);
+
   } else {
     llvm_unreachable("Unhandled loop condition!");
   }
   return 0;
 }
 
 /// \brief Handle a back edge (loop)
 void SIAnnotateControlFlow::handleLoop(BranchInst *Term) {
   BasicBlock *BB = Term->getParent();
   llvm::Loop *L = LI->getLoopFor(BB);
   BasicBlock *Target = Term->getSuccessor(1);
   PHINode *Broken = PHINode::Create(Int64, 0, "", &Target->front());
 
   Value *Cond = Term->getCondition();
   Term->setCondition(BoolTrue);
-  Value *Arg = handleLoopCondition(Cond, Broken, L);
+  Value *Arg = handleLoopCondition(Cond, Broken, L, Term);
 
   for (pred_iterator PI = pred_begin(Target), PE = pred_end(Target);
        PI != PE; ++PI) {
 
     Broken->addIncoming(*PI == BB ? Arg : Int64Zero, *PI);
   }
 
   Term->setCondition(CallInst::Create(Loop, Arg, "", Term));

llvm/trunk/test/CodeGen/AMDGPU/si-annotate-cfg-loop-assert.ll

+; RUN: llc -march=amdgcn -mcpu=kaveri  < %s | FileCheck %s
+
+; CHECK-LABEL: {{^}}test:
+; CHECK s_and_saveexec_b64
+; CHECK s_xor_b64
+; CHECK s_or_b64 exec, exec
+; CHECK s_andn2_b64 exec, exec
+; CHECK s_cbranch_execnz
+define spir_kernel void @test(i32 %arg, i32 %arg1, i32 addrspace(1)* nocapture %arg2, i32 %arg3, i32 %arg4, i32 %arg5, i32 %arg6) {
+bb:
+  %tmp = icmp ne i32 %arg, 0
+  %tmp7 = icmp ne i32 %arg1, 0
+  %tmp8 = and i1 %tmp, %tmp7
+  br i1 %tmp8, label %bb9, label %bb11
+
+bb9:                                              ; preds = %bb
+  br label %bb10
+
+bb10:                                             ; preds = %bb10, %bb9
+  br label %bb10
+
+bb11:                                             ; preds = %bb
+  ret void
+}