[PATCH] D13717: [IndVars] Have `cloneArithmeticIVUser` guess better

[Philip Reames via llvm-commits]

reames added inline comments.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:992
@@ +991,3 @@
+  DEBUG(dbgs() << "Cloning Arithmetic IVUser: " << *DU.NarrowUse << "\n");
+  Instruction *NarrowUse = DU.NarrowUse;
+  Instruction *NarrowDef = DU.NarrowDef;
----------------
Pulling out these variables is an NFC change.  Can you separate and commit?  It'll make the diff easier.

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1001
@@ +1000,3 @@
+
+    auto GetExtend = SignExt ? &ScalarEvolution::getSignExtendExpr
+                             : &ScalarEvolution::getZeroExtendExpr;
----------------
This would be more clear as a lambda which captures SE

Actually, doesn't SCEV have a GetExtend w/a signed param?  I know IR builder does.  Adding one wouldn't be unreasonable.  

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1020
@@ +1019,3 @@
+
+    case Instruction::Add:
+      WideOp = SE->getAddExpr(WideLHS, WideRHS);
----------------
Isn't there something along the lines of SE->getBinOp?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1037
@@ +1036,3 @@
+
+    return WideOp == WideAR;
+  };
----------------
Comment?  Where'd WideAR come from?

================
Comment at: lib/Transforms/Scalar/IndVarSimplify.cpp:1056
@@ -1005,5 +1055,3 @@
 
-  auto *NarrowBO = cast<BinaryOperator>(DU.NarrowUse);
-  auto *WideBO = BinaryOperator::Create(NarrowBO->getOpcode(), LHS, RHS,
-                                        NarrowBO->getName());
-  IRBuilder<> Builder(DU.NarrowUse);
+  BinaryOperator *NarrowBO = cast<BinaryOperator>(DU.NarrowUse);
+  BinaryOperator *WideBO = BinaryOperator::Create(NarrowBO->getOpcode(), LHS,
----------------
Why not use auto here?


http://reviews.llvm.org/D13717