[IndVars] Split `WidenIV::cloneIVUser`; NFC

[sanjoy]

Summary

This NFC splitting is intended to make a later diff easier to follow.
It just tail duplicates cloneIVUser into cloneArithmeticIVUser and
cloneBitwiseIVUser.

[reames]

I'm a bit confused by the need/desirability of this change. Isn't it fairly simple to make the special case for widening conditional on the operator type within a single function? Or is there something more fundamental I'm missing?

[sanjoy]

In D13716#267189, @reames wrote:

I'm a bit confused by the need/desirability of this change. Isn't it fairly simple to make the special case for widening conditional on the operator type within a single function? Or is there something more fundamental I'm missing?

Firstly, the structure of cloneArithmeticIVUser will end up being a little different from cloneBitwiseIVUser; and I think having an "early dispatch" will make it easier to avoid bugs. This is the practical reason why I split the two.

There is a more fundamental difference between the two as well, which I've not drawn attention to here, because it isn't being used -- SCEV is usually to unable to analyze bitwise operations like x & y (unless they're just optimized arithmetic). This means we don't widen uses of IVs like and %iv, 42 (and insert a trunc) even though bitwise operations are (almost by definition) easy to widen (zext (and %iv, 42) == and zext(%iv), zext(42)). -instcombine is usually able to clean these (the trunc s) up, but if we later find interesting cases that -instcombine cannot clean up then we could make cloneBitwiseIVUser smarter, which would further diverge the implementations.

[reames]

LGTM

llvm/trunk/lib/Transforms/Scalar/IndVarSimplify.cpp

public:
   }
 
   PHINode *createWideIV(SCEVExpander &Rewriter);
 
 protected:
   Value *getExtend(Value *NarrowOper, Type *WideType, bool IsSigned,
                    Instruction *Use);
 
-  Instruction *cloneIVUser(NarrowIVDefUse DU);
+  Instruction *cloneIVUser(NarrowIVDefUse DU, const SCEVAddRecExpr *WideAR);
+  Instruction *cloneArithmeticIVUser(NarrowIVDefUse DU,
+                                     const SCEVAddRecExpr *WideAR);
+  Instruction *cloneBitwiseIVUser(NarrowIVDefUse DU);
 
   const SCEVAddRecExpr *getWideRecurrence(Instruction *NarrowUse);
 
   const SCEVAddRecExpr* getExtendedOperandRecurrence(NarrowIVDefUse DU);
 
   const SCEV *getSCEVByOpCode(const SCEV *LHS, const SCEV *RHS,
                               unsigned OpCode) const;
 
Value *WidenIV::getExtend(Value *NarrowOper, Type *WideType, bool IsSigned,
 
   return IsSigned ? Builder.CreateSExt(NarrowOper, WideType) :
                     Builder.CreateZExt(NarrowOper, WideType);
 }
 
 /// Instantiate a wide operation to replace a narrow operation. This only needs
 /// to handle operations that can evaluation to SCEVAddRec. It can safely return
 /// 0 for any operation we decide not to clone.
-Instruction *WidenIV::cloneIVUser(NarrowIVDefUse DU) {
+Instruction *WidenIV::cloneIVUser(NarrowIVDefUse DU,
+                                  const SCEVAddRecExpr *WideAR) {
   unsigned Opcode = DU.NarrowUse->getOpcode();
   switch (Opcode) {
   default:
     return nullptr;
   case Instruction::Add:
   case Instruction::Mul:
   case Instruction::UDiv:
   case Instruction::Sub:
+    return cloneArithmeticIVUser(DU, WideAR);
+
   case Instruction::And:
   case Instruction::Or:
   case Instruction::Xor:
   case Instruction::Shl:
   case Instruction::LShr:
   case Instruction::AShr:
-    DEBUG(dbgs() << "Cloning IVUser: " << *DU.NarrowUse << "\n");
+    return cloneBitwiseIVUser(DU);
+  }
+}
+
+Instruction *WidenIV::cloneBitwiseIVUser(NarrowIVDefUse DU) {
+  DEBUG(dbgs() << "Cloning bitwise IVUser: " << *DU.NarrowUse << "\n");
 
-    // Replace NarrowDef operands with WideDef. Otherwise, we don't know
-    // anything about the narrow operand yet so must insert a [sz]ext. It is
-    // probably loop invariant and will be folded or hoisted. If it actually
-    // comes from a widened IV, it should be removed during a future call to
-    // widenIVUse.
-    Value *LHS = (DU.NarrowUse->getOperand(0) == DU.NarrowDef) ? DU.WideDef :
-      getExtend(DU.NarrowUse->getOperand(0), WideType, IsSigned, DU.NarrowUse);
-    Value *RHS = (DU.NarrowUse->getOperand(1) == DU.NarrowDef) ? DU.WideDef :
-      getExtend(DU.NarrowUse->getOperand(1), WideType, IsSigned, DU.NarrowUse);
+  // Replace NarrowDef operands with WideDef. Otherwise, we don't know anything
+  // about the narrow operand yet so must insert a [sz]ext. It is probably loop
+  // invariant and will be folded or hoisted. If it actually comes from a
+  // widened IV, it should be removed during a future call to widenIVUse.
+  Value *LHS = (DU.NarrowUse->getOperand(0) == DU.NarrowDef)
+                   ? DU.WideDef
+                   : getExtend(DU.NarrowUse->getOperand(0), WideType, IsSigned,
+                               DU.NarrowUse);
+  Value *RHS = (DU.NarrowUse->getOperand(1) == DU.NarrowDef)
+                   ? DU.WideDef
+                   : getExtend(DU.NarrowUse->getOperand(1), WideType, IsSigned,
+                               DU.NarrowUse);
 
-    auto *NarrowBO = cast<BinaryOperator>(DU.NarrowUse);
-    auto *WideBO = BinaryOperator::Create(NarrowBO->getOpcode(), LHS, RHS,
-                                          NarrowBO->getName());
-    IRBuilder<> Builder(DU.NarrowUse);
-    Builder.Insert(WideBO);
-    if (const auto *OBO = dyn_cast<OverflowingBinaryOperator>(NarrowBO)) {
-      if (OBO->hasNoUnsignedWrap()) WideBO->setHasNoUnsignedWrap();
-      if (OBO->hasNoSignedWrap()) WideBO->setHasNoSignedWrap();
-    }
-    return WideBO;
-  }
+    if (OBO->hasNoUnsignedWrap())
+      WideBO->setHasNoUnsignedWrap();
+    if (OBO->hasNoSignedWrap())
+      WideBO->setHasNoSignedWrap();
+
+Instruction *WidenIV::cloneArithmeticIVUser(NarrowIVDefUse DU,
+                                            const SCEVAddRecExpr *WideAR) {
+  DEBUG(dbgs() << "Cloning arithmetic IVUser: " << *DU.NarrowUse << "\n");
+
+  // Replace NarrowDef operands with WideDef. Otherwise, we don't know anything
+  // about the narrow operand yet so must insert a [sz]ext. It is probably loop
+  // invariant and will be folded or hoisted. If it actually comes from a
+  // widened IV, it should be removed during a future call to widenIVUse.
+  Value *LHS = (DU.NarrowUse->getOperand(0) == DU.NarrowDef)
+                   ? DU.WideDef
+                   : getExtend(DU.NarrowUse->getOperand(0), WideType, IsSigned,
+                               DU.NarrowUse);
+  Value *RHS = (DU.NarrowUse->getOperand(1) == DU.NarrowDef)
+                   ? DU.WideDef
+                   : getExtend(DU.NarrowUse->getOperand(1), WideType, IsSigned,
+                               DU.NarrowUse);
+
+  auto *NarrowBO = cast<BinaryOperator>(DU.NarrowUse);
+  auto *WideBO = BinaryOperator::Create(NarrowBO->getOpcode(), LHS, RHS,
+                                        NarrowBO->getName());
+  IRBuilder<> Builder(DU.NarrowUse);
+  Builder.Insert(WideBO);
+  if (const auto *OBO = dyn_cast<OverflowingBinaryOperator>(NarrowBO)) {
+    if (OBO->hasNoUnsignedWrap())
+      WideBO->setHasNoUnsignedWrap();
+    if (OBO->hasNoSignedWrap())
+      WideBO->setHasNoSignedWrap();
+  }
+  return WideBO;
 }
 
 const SCEV *WidenIV::getSCEVByOpCode(const SCEV *LHS, const SCEV *RHS,
                                      unsigned OpCode) const {
   if (OpCode == Instruction::Add)
     return SE->getAddExpr(LHS, RHS);
   if (OpCode == Instruction::Sub)
     return SE->getMinusSCEV(LHS, RHS);
Instruction *WidenIV::widenIVUse(NarrowIVDefUse DU, SCEVExpander &Rewriter) {
 
   // Reuse the IV increment that SCEVExpander created as long as it dominates
   // NarrowUse.
   Instruction *WideUse = nullptr;
   if (WideAddRec == WideIncExpr
       && Rewriter.hoistIVInc(WideInc, DU.NarrowUse))
     WideUse = WideInc;
   else {
-    WideUse = cloneIVUser(DU);
+    WideUse = cloneIVUser(DU, WideAddRec);
     if (!WideUse)
       return nullptr;
   }
   // Evaluation of WideAddRec ensured that the narrow expression could be
   // extended outside the loop without overflow. This suggests that the wide use
   // evaluates to the same expression as the extended narrow use, but doesn't
   // absolutely guarantee it. Hence the following failsafe check. In rare cases
   // where it fails, we simply throw away the newly created wide use.