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

[Jingyue Wu]

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:271
@@ +270,3 @@
+      BO->getOpcode() == Instruction::Xor) {
+    return true;
+  }
----------------
Mark Heffernan wrote:
> Are XOR and AND handled properly?  OR is limited to cases where it is equivalent to add so no problem there, but it seems like hoisting a constant out of a XOR or AND expression would be problematic.
Good catch! Fixed by ignoring XOR and AND. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:330
@@ +329,3 @@
+      // Do not trace into "or" unless it is equivalent to "add". If LHS and
+      // RHS don't have common bits, (LHS | RHS) is equivalent to (LHS + RHS).
+      return APInt(BitWidth, 0);
----------------
Mark Heffernan wrote:
> Can this OR logic be moved up into Distributable or maybe move Distributable logic here?  In general, I found find() a bit tricky to follow and reason about.  Some of it is due to the complexity about the sext/zext identities you have to use, but some it was because the logic for checking whether you can traverse beneath an operator is in more than one place.
Merged the logics for OR, ADD and SUB into CanTraceInto. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:350
@@ +349,3 @@
+      if (ConstantOffset.isNegative())
+        ConstantOffset.clearAllBits(); // ConstantOffset = 0
+    }
----------------
Mark Heffernan wrote:
> Is UserChain updated properly in the case where the constant is negative?  Seems like you could have some elements added to UserChain in the call to findInEitherOperand above then it hits this condition and returns 0 and searching begins along another branch in the expression with stale elements in UserChain.
> 
> Also, if ConstOffset is nonnegative does that necessarily guarantee that one of the operands of the add is nonnegative?  The constant could be down more than one level in the expression, right?
Good catch * 2! 

The new code only traces into sext(a + b) (without nsw) if a + b >= 0 and one of a and b is non-negative. Therefore, we needn't worry about restoring UserChain any more. 

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:396
@@ +395,3 @@
+    return Constant::getNullValue(ExtInsts.empty() ? U->getType()
+                                                   : ExtInsts[0]->getType());
+  }
----------------
Mark Heffernan wrote:
> Why is the type of ExtInsts[0] used here rather than the last element of ExtInsts which would be closest to the constant?
We distribute s/zext all the way down UserChain, so the type should be the outmost type. 

e.g., after distributing s/zext, zext(sext(a + (b + 5)) (assuming no overflow) becomes zext(sext(a)) + (zext(sext(b)) + zext(sext(5))). Then, removing constant offset 5 leaves zext(sext(a)) + (zext(sext(b)) + zext(sext(0))). The type of zext(sext(0)) is the type of the outmost zext. 

To make the logic of rebuildWithoutConstOffset clearer, I split it into two steps: distributing s/zext and removing the constant offset. Although the code may run slower (two passes instead of one), the logic should be much simpler to follow. PTAL

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:415
@@ +414,3 @@
+
+  // If NextInChain is 0 and TheOther is the RHS of a sub, we can
+  // simplify the sub-expression to be just TheOther.
----------------
Mark Heffernan wrote:
> Should be: ...is *not* the RHS of a sub.  Also, I think it'd be a bit clearer if your logic was in the same form as your comment like so:
> 
> if (I->isZero() && !(BO->getOpcode() == Instruction::Sub && OpNo == 0)) {
> 
> Or alternatively change the comment logic.
You're right. Done

http://reviews.llvm.org/D3985