Reapply chandlerc's r173342 now that the miscompile it was triggering is fixed.

Original commit message:
Plug TTI into the speculation logic, giving it a real cost interface
that can be specialized by targets.

The goal here is not to be more aggressive, but to just be more accurate
with very obvious cases. There are instructions which are known to be
truly free and which were not being modeled as such in this code -- see
the regression test which is distilled from an inner loop of zlib.

Everywhere the TTI cost model is insufficiently conservative I've added
explicit checks with FIXME comments to go add proper modelling of these
cost factors.

If this causes regressions, the likely solution is to make TTI even more
conservative in its cost estimates, but test cases will help here.

llvm-svn: 173357

diff --git a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
index 11cb25d0f973a8712f3ae2f68ed8df5e5e0858e2..9f3464de43028716297fc112f3ef1fbd080ef574 100644 (file)
--- a/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
+++ b/llvm/lib/Transforms/Utils/SimplifyCFG.cpp
@@ -1369,7 +1369,8 @@ static bool SinkThenElseCodeToEnd(BranchInst *BI1) {
 /// \endcode
 ///
 /// \returns true if the conditional block is removed.
-static bool SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB) {
+static bool SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB,
+                                   const TargetTransformInfo &TTI) {
   // Be conservative for now. FP select instruction can often be expensive.
   Value *BrCond = BI->getCondition();
   if (isa<FCmpInst>(BrCond))
@@ -1398,15 +1399,22 @@ static bool SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB) {
 
     // Only speculatively execution a single instruction (not counting the
     // terminator) for now.
-    ++SpeculationCost;
-    if (SpeculationCost > 1)
+    SpeculationCost += TTI.getUserCost(I);
+    if (SpeculationCost > TargetTransformInfo::TCC_Basic)
       return false;
 
     // Don't hoist the instruction if it's unsafe or expensive.
     if (!isSafeToSpeculativelyExecute(I))
       return false;
-    if (ComputeSpeculationCost(I) > PHINodeFoldingThreshold)
+    // FIXME: This should really be a cost metric, but our cost model doesn't
+    // accurately model the expense of select.
+    if (isa<SelectInst>(I))
       return false;
+    // FIXME: The cost metric currently doesn't reason accurately about simple
+    // versus complex GEPs, take a conservative approach here.
+    if (GEPOperator *GEP = dyn_cast<GEPOperator>(I))
+      if (!GEP->hasAllConstantIndices())
+        return false;
 
     // Do not hoist the instruction if any of its operands are defined but not
     // used in this BB. The transformation will prevent the operand from
@@ -1449,9 +1457,10 @@ static bool SpeculativelyExecuteBB(BranchInst *BI, BasicBlock *ThenBB) {
     // Account for the cost of an unfolded ConstantExpr which could end up
     // getting expanded into Instructions.
     // FIXME: This doesn't account for how many operations are combined in the
-    // constant expression.
-    ++SpeculationCost;
-    if (SpeculationCost > 1)
+    // constant expression. The cost functions in TTI don't yet correctly model
+    // constant expression costs.
+    SpeculationCost += TargetTransformInfo::TCC_Basic;
+    if (SpeculationCost > TargetTransformInfo::TCC_Basic)
       return false;
   }
 
@@ -3868,7 +3877,7 @@ bool SimplifyCFGOpt::SimplifyCondBranch(BranchInst *BI, IRBuilder<> &Builder) {
       TerminatorInst *Succ0TI = BI->getSuccessor(0)->getTerminator();
       if (Succ0TI->getNumSuccessors() == 1 &&
           Succ0TI->getSuccessor(0) == BI->getSuccessor(1))
-        if (SpeculativelyExecuteBB(BI, BI->getSuccessor(0)))
+        if (SpeculativelyExecuteBB(BI, BI->getSuccessor(0), TTI))
           return SimplifyCFG(BB, TTI, TD) | true;
     }
   } else if (BI->getSuccessor(1)->getSinglePredecessor() != 0) {
@@ -3877,7 +3886,7 @@ bool SimplifyCFGOpt::SimplifyCondBranch(BranchInst *BI, IRBuilder<> &Builder) {
     TerminatorInst *Succ1TI = BI->getSuccessor(1)->getTerminator();
     if (Succ1TI->getNumSuccessors() == 1 &&
         Succ1TI->getSuccessor(0) == BI->getSuccessor(0))
-      if (SpeculativelyExecuteBB(BI, BI->getSuccessor(1)))
+      if (SpeculativelyExecuteBB(BI, BI->getSuccessor(1), TTI))
         return SimplifyCFG(BB, TTI, TD) | true;
   }
 

diff --git a/llvm/test/Transforms/SimplifyCFG/SpeculativeExec.ll b/llvm/test/Transforms/SimplifyCFG/SpeculativeExec.ll
index 7e27f415f76694da42c22a40682a6e535a2db12a..bcef8480405c7a4dfa6f16e76ae0b12f34570ee5 100644 (file)
--- a/llvm/test/Transforms/SimplifyCFG/SpeculativeExec.ll
+++ b/llvm/test/Transforms/SimplifyCFG/SpeculativeExec.ll
@@ -108,3 +108,32 @@ end:
 
   ret i8* %x10
 }
+
+define i16 @test5(i1* %dummy, i16 %a, i16 %b) {
+; Test that we speculate no-op instructions.
+; CHECK: @test5
+
+entry:
+  %cond1 = load volatile i1* %dummy
+  br i1 %cond1, label %if, label %end
+
+if:
+  %cond2 = load volatile i1* %dummy
+  %a.conv = sext i16 %a to i32
+  %b.conv = sext i16 %b to i32
+  %cmp = icmp ult i32 %a.conv, %b.conv
+  br i1 %cond2, label %then, label %end
+
+then:
+  %sub = sub i32 %a.conv, %b.conv
+  %sub.conv = trunc i32 %sub to i16
+  br label %end
+
+end:
+  %x = phi i16 [ %a, %entry ], [ %b, %if ], [ %sub.conv, %then ]
+; CHECK-NOT: phi
+; CHECK: select i1
+
+  ret i16 %x
+}
+