[CGP] Consolidate logic for getIVIncrement and isIVIncrement

This fixes the bug demonstrated by the test case in the commit message of 8d20f2c2 (which was a revert of cf82700).  The root issue was that we have two transforms which are inverses of each other.  We use one for simple induction variables (where we can use the post-inc form), and the other for everything else.  The problem was that the two transforms could disagree about whether something was an induction variable.

The reverted commit made a change to one of the matcher routines which was used for one of the two transforms without updating the other matcher.  However, it's worth noting the existing code w/o the reverted change also has cases where the decision could differ between the two paths.

The fix is simply to consolidate the code such that two paths must agree by construction, and to add an assert to catch any potential future re-divergence.

Triggering the infinite loop requires side stepping the SunkAddrs cache.  The SunkAddrs cache has the effect of suppressing the iteration in the common case, but there are codepaths through CGP which restart iteration and clear this cache.

Unfortunately, I have not been able to construct a standalone IR test case for this.  The original test case is a c++ program which when compiled by clang demonstrates the infinite loop, but all of my attempts at extracting an IR test case runnable through opt/llc have failed to reproduce.  (Including capturing the IR at point of the transform itself!)  I have no idea what weird state clang is creating here.

I also tried creating a test case by hand, but gave up after about an hour of trying to find the right combination to dance through multiple transforms to create the end result needed to trip the bug.

diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
index 0b1156e2ace7792de7b556fa7dc0ab389cf0a5a6..0182cdf2ca8cb34e77d09deea706c0abde69520b 100644 (file)
--- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
+++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
@@ -1304,6 +1304,24 @@ static bool OptimizeNoopCopyExpression(CastInst *CI, const TargetLowering &TLI,
   return SinkCast(CI);
 }
 
+// Match a simple increment by constant operation.  Note that if a sub is
+// matched, the step is negated (as if the step had been canonicalized to
+// an add, even though we leave the instruction alone.)
+bool matchIncrement(const Instruction* IVInc, Instruction *&LHS,
+                    Constant *&Step) {
+  if (match(IVInc, m_Add(m_Instruction(LHS), m_Constant(Step))) ||
+      match(IVInc, m_ExtractValue<0>(m_Intrinsic<Intrinsic::uadd_with_overflow>(
+                       m_Instruction(LHS), m_Constant(Step)))))
+    return true;
+  if (match(IVInc, m_Sub(m_Instruction(LHS), m_Constant(Step))) ||
+      match(IVInc, m_ExtractValue<0>(m_Intrinsic<Intrinsic::usub_with_overflow>(
+                       m_Instruction(LHS), m_Constant(Step))))) {
+    Step = ConstantExpr::getNeg(Step);
+    return true;
+  }
+  return false;
+}
+
 /// If given \p PN is an inductive variable with value IVInc coming from the
 /// backedge, and on each iteration it gets increased by Step, return pair
 /// <IVInc, Step>. Otherwise, return None.
@@ -1316,26 +1334,24 @@ getIVIncrement(const PHINode *PN, const LoopInfo *LI) {
       dyn_cast<Instruction>(PN->getIncomingValueForBlock(L->getLoopLatch()));
   if (!IVInc)
     return None;
+  Instruction *LHS = nullptr;
   Constant *Step = nullptr;
-  if (match(IVInc, m_Sub(m_Specific(PN), m_Constant(Step))))
-    return std::make_pair(IVInc, ConstantExpr::getNeg(Step));
-  if (match(IVInc, m_Add(m_Specific(PN), m_Constant(Step))))
-    return std::make_pair(IVInc, Step);
-  if (match(IVInc, m_ExtractValue<0>(m_Intrinsic<Intrinsic::usub_with_overflow>(
-                       m_Specific(PN), m_Constant(Step)))))
-    return std::make_pair(IVInc, ConstantExpr::getNeg(Step));
-  if (match(IVInc, m_ExtractValue<0>(m_Intrinsic<Intrinsic::uadd_with_overflow>(
-                       m_Specific(PN), m_Constant(Step)))))
+  if (matchIncrement(IVInc, LHS, Step) && LHS == PN)
     return std::make_pair(IVInc, Step);
   return None;
 }
 
-static bool isIVIncrement(const BinaryOperator *BO, const LoopInfo *LI) {
-  auto *PN = dyn_cast<PHINode>(BO->getOperand(0));
-  if (!PN)
+static bool isIVIncrement(const Value *V, const LoopInfo *LI) {
+  auto *I = dyn_cast<Instruction>(V);
+  if (!I)
+    return false;
+  Instruction *LHS = nullptr;
+  Constant *Step = nullptr;
+  if (!matchIncrement(I, LHS, Step))
     return false;
-  if (auto IVInc = getIVIncrement(PN, LI))
-    return IVInc->first == BO;
+  if (auto *PN = dyn_cast<PHINode>(LHS))
+    if (auto IVInc = getIVIncrement(PN, LI))
+      return IVInc->first == I;
   return false;
 }
 
@@ -3861,8 +3877,7 @@ bool AddressingModeMatcher::matchScaledValue(Value *ScaleReg, int64_t Scale,
   ConstantInt *CI = nullptr; Value *AddLHS = nullptr;
   if (isa<Instruction>(ScaleReg) && // not a constant expr.
       match(ScaleReg, m_Add(m_Value(AddLHS), m_ConstantInt(CI))) &&
-      !isIVIncrement(cast<BinaryOperator>(ScaleReg), &LI) &&
-      CI->getValue().isSignedIntN(64)) {
+      !isIVIncrement(ScaleReg, &LI) && CI->getValue().isSignedIntN(64)) {
     TestAddrMode.InBounds = false;
     TestAddrMode.ScaledReg = AddLHS;
     TestAddrMode.BaseOffs += CI->getSExtValue() * TestAddrMode.Scale;
@@ -3878,6 +3893,8 @@ bool AddressingModeMatcher::matchScaledValue(Value *ScaleReg, int64_t Scale,
     TestAddrMode = AddrMode;
   }
 
+  // If this is an add recurrence with a constant step, return the increment
+  // instruction and the canonicalized step.
   auto GetConstantStep = [this](const Value * V)
       ->Optional<std::pair<Instruction *, APInt> > {
     auto *PN = dyn_cast<PHINode>(V);
@@ -3914,6 +3931,11 @@ bool AddressingModeMatcher::matchScaledValue(Value *ScaleReg, int64_t Scale,
   if (AddrMode.BaseOffs) {
     if (auto IVStep = GetConstantStep(ScaleReg)) {
       Instruction *IVInc = IVStep->first;
+      // The following assert is important to ensure a lack of infinite loops.
+      // This transforms is (intentionally) the inverse of the one just above.
+      // If they don't agree on the definition of an increment, we'd alternate
+      // back and forth indefinitely.
+      assert(isIVIncrement(IVInc, &LI) && "implied by GetConstantStep");
       APInt Step = IVStep->second;
       APInt Offset = Step * AddrMode.Scale;
       if (Offset.isSignedIntN(64)) {