[PATCH] Fixed several correctness issues on mishandling s/zext in SeparateConstOffsetFromGEP

[Mark Heffernan]

LGTM.  I like the restructuring of the traversal logic.  It's easier to follow now.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:292
@@ +291,3 @@
+    // If a + b >= 0 and (a >= 0 or b >= 0), then
+    //   sext(a + b) = sext(a) + sext(b)
+    // even if the addition is not marked nsw.
----------------
True for zext as well?  If so, please add to comment.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:419
@@ -350,20 +418,3 @@
 
-Value *ConstantOffsetExtractor::rebuildLeafWithoutConstantOffset(User *U,
-                                                                 Value *C) {
-  assert(U->getNumOperands() <= 2 &&
-         "We didn't trace into any operator with more than 2 operands");
-  // If U has only one operand which is the constant offset, removing the
-  // constant offset leaves U as a null value.
-  if (U->getNumOperands() == 1)
-    return Constant::getNullValue(U->getType());
-
-  // U->getNumOperands() == 2
-  unsigned OpNo = FindFirstUse(U, C); // U->getOperand(OpNo) == C
-  assert(OpNo < 2 && "UserChain wasn't built correctly");
-  Value *TheOther = U->getOperand(1 - OpNo); // The other operand of U
-  // If U = C - X, removing C makes U = -X; otherwise U will simply be X.
-  if (!isa<SubOperator>(U) || OpNo == 1)
-    return TheOther;
-  if (isa<ConstantExpr>(U))
-    return ConstantExpr::getNeg(cast<Constant>(TheOther));
-  return BinaryOperator::CreateNeg(TheOther, "", IP);
+Value *ConstantOffsetExtractor::distributeExts(unsigned ChainIndex) {
+  User *U = UserChain[ChainIndex];
----------------
nit: maybe call function distributeExtsAndCloneChain.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:492
@@ +491,3 @@
+  // instruction type and BO is used at most once.
+  assert(BO->getNumUses() <= 1 &&
+         "distributeExts clones each BinaryOperator in "
----------------
Can number of users ever be zero?  That is, can't you assert B0->getNumUses() == 1?

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:493
@@ +492,3 @@
+  assert(BO->getNumUses() <= 1 &&
+         "distributeExts clones each BinaryOperator in "
+         "UserChain, so no one should be used more than "
----------------
If you change the name of distributeExt, you'll have to change it here as well.

http://reviews.llvm.org/D3985