[GuardWidening] Ignore guards with trivial conditions

[mkazantsev]

Summary

Guard widening should not spend efforts on dealing with guards with trivial true/false conditions.
Such guards can easily be eliminated by any further cleanup pass like instcombine. However we
should not unconditionally delete them because it may be profitable to widen other conditions
into such guards.

[fedor.sergeev]

LGTM bar some minor stylistic stuff.

lib/Transforms/Scalar/GuardWidening.cpp

354

Can we just check that it is constant? Booleans hardly get any values other than True/False...

422

Uh... that reads somewhat uneasy, as False/True values are way up and I first read them as just bools :)

lib/Transforms/Scalar/GuardWidening.cpp

bool GuardWideningImpl::run() {
 
   return Changed;
 }
 
 bool GuardWideningImpl::eliminateGuardViaWidening(
     Instruction *GuardInst, const df_iterator<DomTreeNode *> &DFSI,
     const DenseMap<BasicBlock *, SmallVector<Instruction *, 8>> &
         GuardsInBlock, bool InvertCondition) {
+  auto *Cond = getCondition(GuardInst);
+  auto *True = ConstantInt::getTrue(GuardInst->getContext());
+  auto *False = ConstantInt::getFalse(GuardInst->getContext());
+  // Ignore trivial true or false conditions. These instructions will be
+  // trivially eliminated by any cleanup pass. Do not erase them because other
+  // guards can possibly be widened into them.
+  if (Cond == True || Cond == False)
+    return false;
+
   Instruction *BestSoFar = nullptr;
   auto BestScoreSoFar = WS_IllegalOrNegative;
   auto *GuardInstLoop = LI.getLoopFor(GuardInst->getParent());
 
   // In the set of dominating guards, find the one we can merge GuardInst with
   // for the most profit.
   for (unsigned i = 0, e = DFSI.getPathLength(); i != e; ++i) {
     auto *CurBB = DFSI.getPath(i)->getBlock();
if (i == (e - 1) && CurBB->getTerminator() != GuardInst) {
       auto NewEnd = std::find(I, E, GuardInst);
       assert(NewEnd != E && "GuardInst not in its own block?");
       E = NewEnd;
     }
 
     for (auto *Candidate : make_range(I, E)) {
       auto Score =
           computeWideningScore(GuardInst, GuardInstLoop, Candidate, CurLoop);
-      LLVM_DEBUG(dbgs() << "Score between " << *getCondition(GuardInst)
+      LLVM_DEBUG(dbgs() << "Score between " << *Cond
                         << " and " << *getCondition(Candidate) << " is "
                         << scoreTypeToString(Score) << "\n");
       if (Score > BestScoreSoFar) {
         BestScoreSoFar = Score;
         BestSoFar = Candidate;
       }
     }
   }
 
   if (BestScoreSoFar == WS_IllegalOrNegative) {
     LLVM_DEBUG(dbgs() << "Did not eliminate guard " << *GuardInst << "\n");
     return false;
   }
 
   assert(BestSoFar != GuardInst && "Should have never visited same guard!");
   assert(DT.dominates(BestSoFar, GuardInst) && "Should be!");
 
   LLVM_DEBUG(dbgs() << "Widening " << *GuardInst << " into " << *BestSoFar
                     << " with score " << scoreTypeToString(BestScoreSoFar)
                     << "\n");
   widenGuard(BestSoFar, getCondition(GuardInst), InvertCondition);
-  auto NewGuardCondition = InvertCondition
-                               ? ConstantInt::getFalse(GuardInst->getContext())
-                               : ConstantInt::getTrue(GuardInst->getContext());
+  auto *NewGuardCondition = InvertCondition ? False : True;
   setCondition(GuardInst, NewGuardCondition);
   EliminatedGuardsAndBranches.push_back(GuardInst);
   WidenedGuards.insert(BestSoFar);
   return true;
 }
 
 GuardWideningImpl::WideningScore GuardWideningImpl::computeWideningScore(
     Instruction *DominatedGuard, Loop *DominatedGuardLoop,

test/Transforms/GuardWidening/basic.ll

; CHECK:  call void (i1, ...) @llvm.experimental.guard(i1 %cond_1) [ "deopt"() ]
 
   %cond_1 = icmp sgt i32 %a, 10
   call void(i1, ...) @llvm.experimental.guard(i1 %cond_1) [ "deopt"() ]
   ret void
 
 right:
   ret void
 }
+
+; Make sure we do not widen guard by trivial true conditions into something.
+define void @f_15(i1 %cond_0, i1 %cond_1) {
+; CHECK-LABEL: @f_15(
+entry:
+; CHECK:  call void (i1, ...) @llvm.experimental.guard(i1 %cond_0) [ "deopt"() ]
+; CHECK:  call void (i1, ...) @llvm.experimental.guard(i1 true) [ "deopt"() ]
+; CHECK:  ret void
+
+  call void(i1, ...) @llvm.experimental.guard(i1 %cond_0) [ "deopt"() ]
+  call void(i1, ...) @llvm.experimental.guard(i1 true) [ "deopt"() ]
+  ret void
+}
+
+; Make sure we do not widen guard by trivial false conditions into something.
+define void @f_16(i1 %cond_0, i1 %cond_1) {
+; CHECK-LABEL: @f_16(
+entry:
+; CHECK:  call void (i1, ...) @llvm.experimental.guard(i1 %cond_0) [ "deopt"() ]
+; CHECK:  call void (i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+; CHECK:  ret void
+
+  call void(i1, ...) @llvm.experimental.guard(i1 %cond_0) [ "deopt"() ]
+  call void(i1, ...) @llvm.experimental.guard(i1 false) [ "deopt"() ]
+  ret void
+}