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

Mark Heffernan meheff at google.com
Tue Jun 3 23:44:44 PDT 2014


Looks pretty good, in general.  It was a bit tough to reason about in places, but I think that had to do with the complexity of wrapping my head around  how sext/zext work across operators as much as anything.  I don't have great suggestions for making it significantly simpler.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:271
@@ +270,3 @@
+      BO->getOpcode() == Instruction::Xor) {
+    return true;
+  }
----------------
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.

================
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);
----------------
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.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:350
@@ +349,3 @@
+      if (ConstantOffset.isNegative())
+        ConstantOffset.clearAllBits(); // ConstantOffset = 0
+    }
----------------
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?

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:396
@@ +395,3 @@
+    return Constant::getNullValue(ExtInsts.empty() ? U->getType()
+                                                   : ExtInsts[0]->getType());
+  }
----------------
Why is the type of ExtInsts[0] used here rather than the last element of ExtInsts which would be closest to the constant?

================
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.
----------------
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.

http://reviews.llvm.org/D3985






More information about the llvm-commits mailing list