[SimplifyCFG] Allow hoisting terminators only with HoistCommonInsts=false.

[fhahn]

Summary

As a side-effect of the change to default HoistCommonInsts to false
early in the pipeline, we fail to convert conditional branch & phis to
selects early on, which prevents vectorization for loops that contain
conditional branches that effectively are selects (or if the loop gets
vectorized, it will get vectorized very inefficiently).

This patch updates SimplifyCFG to perform hoisting if the only
instruction in both BBs is an equal branch. In this case, the only
additional instructions are selects for phis, which should be cheap.

Even though we perform hoisting, the benefits of this kind of hoisting
should by far outweigh the negatives.

For example, the loop in the code below will not get vectorized on
AArch64 with the current default, but will with the patch. This is a
fundamental pattern we should definitely vectorize. Besides that, I
think the select variants should be easier to use for reasoning across
other passes as well.

https://clang.godbolt.org/z/sbjd8Wshx

double clamp(double v) {
  if (v < 0.0)
    return 0.0;
  if (v > 6.0)
    return 6.0;
  return v;
}

void loop(double* X, double *Y) {
  for (unsigned i = 0; i < 20000; i++) {
    X[i] = clamp(Y[i]);
  }
}

[fhahn]

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

1439

This could be further restricted to unconditional branches only, to avoid cases with many new successors with PHI nodes.

[lebedev.ri]

Please add phaseordering test

[fhahn]

Add phase-ordering test.

[lebedev.ri]

Hmm, this seems pretty harmless.
I do wonder if FoldTwoEntryPHINode() should be generalized, but until then this seems ok.
Thank you.

[fhahn]

In D100329#2683985, @lebedev.ri wrote:

Hmm, this seems pretty harmless.
I do wonder if FoldTwoEntryPHINode() should be generalized, but until then this seems ok.
Thank you.

Thanks! I checked the stats for loop-rotate for -O3 -flto on ARM64, and there were no changes, so it does not seem to work against the original goal of D84108. I'm not sure how much work adjusting FoldTwoEntryPHINode would be and what the benefits would be. One advantage of doing it HoistThenElseCodeToIf is that if there are no PHIs, there are no extra instructions necessary at all. But if the successors have PHIs, it might make sense to limit it to cases where the phis can be folded.

[uabelho]

Hi,

This patch seems to make -simplifycfg generate different code with/without dbg intrinsics present.
So
opt -simplifycfg -S -o - scfg.ll
and
opt -strip-debug -o - -S scfg.ll | opt -simplifycfg -S -o -
give different results for the attached scfg.ll.

scfg.ll3 KBDownload

[uabelho]

In D100329#2688727, @uabelho wrote:

Hi,

This patch seems to make -simplifycfg generate different code with/without dbg intrinsics present.
So
opt -simplifycfg -S -o - scfg.ll
and
opt -strip-debug -o - -S scfg.ll | opt -simplifycfg -S -o -
give different results for the attached scfg.ll.

scfg.ll3 KBDownload

I wrote https://bugs.llvm.org/show_bug.cgi?id=49982 about it.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp

class SimplifyCFGOpt {
   bool simplifyBranch(BranchInst *Branch, IRBuilder<> &Builder);
   bool simplifyUncondBranch(BranchInst *BI, IRBuilder<> &Builder);
   bool simplifyCondBranch(BranchInst *BI, IRBuilder<> &Builder);
   bool SimplifyCondBranchToTwoReturns(BranchInst *BI, IRBuilder<> &Builder);
 
   bool tryToSimplifyUncondBranchWithICmpInIt(ICmpInst *ICI,
                                              IRBuilder<> &Builder);
 
-  bool HoistThenElseCodeToIf(BranchInst *BI, const TargetTransformInfo &TTI);
+  bool HoistThenElseCodeToIf(BranchInst *BI, const TargetTransformInfo &TTI,
+                             bool EqTermsOnly);
   bool SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
                               const TargetTransformInfo &TTI);
   bool SimplifyTerminatorOnSelect(Instruction *OldTerm, Value *Cond,
                                   BasicBlock *TrueBB, BasicBlock *FalseBB,
                                   uint32_t TrueWeight, uint32_t FalseWeight);
   bool SimplifyBranchOnICmpChain(BranchInst *BI, IRBuilder<> &Builder,
                                  const DataLayout &DL);
   bool SimplifySwitchOnSelect(SwitchInst *SI, SelectInst *Select);
static bool isSafeToHoistInvoke(BasicBlock *BB1, BasicBlock *BB2,
   }
   return true;
 }
 
 static bool passingValueIsAlwaysUndefined(Value *V, Instruction *I, bool PtrValueMayBeModified = false);
 
 /// Given a conditional branch that goes to BB1 and BB2, hoist any common code
 /// in the two blocks up into the branch block. The caller of this function
-/// guarantees that BI's block dominates BB1 and BB2.
+/// guarantees that BI's block dominates BB1 and BB2. If EqTermsOnly is given,
+/// only perform hoisting in case both blocks only contain a terminator. In that
+/// case, only the original BI will be replaced and selects for PHIs are added.
 bool SimplifyCFGOpt::HoistThenElseCodeToIf(BranchInst *BI,
-                                           const TargetTransformInfo &TTI) {
+                                           const TargetTransformInfo &TTI,
+                                           bool EqTermsOnly) {
   // This does very trivial matching, with limited scanning, to find identical
   // instructions in the two blocks.  In particular, we don't want to get into
   // O(M*N) situations here where M and N are the sizes of BB1 and BB2.  As
   // such, we currently just scan for obviously identical instructions in an
   // identical order.
   BasicBlock *BB1 = BI->getSuccessor(0); // The true destination.
   BasicBlock *BB2 = BI->getSuccessor(1); // The false destination
 
bool SimplifyCFGOpt::HoistThenElseCodeToIf(BranchInst *BI,
 
   bool Changed = false;
 
   auto _ = make_scope_exit([&]() {
     if (Changed)
       ++NumHoistCommonCode;
   });
 
+  // Check if only hoisting terminators is allowed. This does not add new
+  // instructions to the hoist location.
+  if (EqTermsOnly) {
+    if (!I1->isIdenticalToWhenDefined(I2))
+      return false;
+    if (!I1->isTerminator())
+      return false;
+    goto HoistTerminator;
+  }
+
   do {
     // If we are hoisting the terminator instruction, don't move one (making a
     // broken BB), instead clone it, and remove BI.
     if (I1->isTerminator())
       goto HoistTerminator;
 
     // If we're going to hoist a call, make sure that the two instructions we're
     // commoning/hoisting are both marked with musttail, or neither of them is
if (FoldBranchToCommonDest(BI, DTU, /*MSSAU=*/nullptr, &TTI,
     return requestResimplify();
 
   // We have a conditional branch to two blocks that are only reachable
   // from BI.  We know that the condbr dominates the two blocks, so see if
   // there is any identical code in the "then" and "else" blocks.  If so, we
   // can hoist it up to the branching block.
   if (BI->getSuccessor(0)->getSinglePredecessor()) {
     if (BI->getSuccessor(1)->getSinglePredecessor()) {
-      if (HoistCommon && Options.HoistCommonInsts)
-        if (HoistThenElseCodeToIf(BI, TTI))
-          return requestResimplify();
+      if (HoistCommon &&
+          HoistThenElseCodeToIf(BI, TTI, !Options.HoistCommonInsts))
     } else {
       // If Successor #1 has multiple preds, we may be able to conditionally
       // execute Successor #0 if it branches to Successor #1.
       Instruction *Succ0TI = BI->getSuccessor(0)->getTerminator();
       if (Succ0TI->getNumSuccessors() == 1 &&
           Succ0TI->getSuccessor(0) == BI->getSuccessor(1))
         if (SpeculativelyExecuteBB(BI, BI->getSuccessor(0), TTI))
           return requestResimplify();

llvm/test/Transforms/SimplifyCFG/common-code-hoisting.ll

for.end:
   call void @f2()
   br label %return
 
 return:
   ret void
 }
 
 ; A example where only the branch instructions from %if.then2 and %if.end3 need
-; to be hoisted, which effectively replaces the original branch in %if.end. As
-; this does not add any new instructions to the hoist location, it should always be profitable.
+; to be hoisted, which effectively replaces the original branch in %if.end and
+; only requires selects for PHIs in the successor.
 define float @clamp_float_value(float %value, float %minimum_value, float %maximum_value) {
 ; HOIST-LABEL: @clamp_float_value(
 ; HOIST-NEXT:  entry:
 ; HOIST-NEXT:    [[CMP:%.*]] = fcmp ogt float [[VALUE:%.*]], [[MAXIMUM_VALUE:%.*]]
 ; HOIST-NEXT:    [[CMP1:%.*]] = fcmp olt float [[VALUE]], [[MINIMUM_VALUE:%.*]]
 ; HOIST-NEXT:    [[MINIMUM_VALUE_VALUE:%.*]] = select i1 [[CMP1]], float [[MINIMUM_VALUE]], float [[VALUE]]
 ; HOIST-NEXT:    [[RETVAL_0:%.*]] = select i1 [[CMP]], float [[MAXIMUM_VALUE]], float [[MINIMUM_VALUE_VALUE]]
 ; HOIST-NEXT:    ret float [[RETVAL_0]]