[PATCH] [SeparateConstOffsetFromGEP] inbounds zext => sext for better splitting

Mark Heffernan meheff at google.com
Sun Jun 8 11:39:22 PDT 2014


LGTM after clarifying and fixing some of the comments.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:285
@@ +284,3 @@
+  ///
+  /// Verfiied in @inbounds_zext_add in split-gep.ll and @sum_of_array3 in
+  /// split-gep-and-gvn.ll
----------------
spelling: Verified.  Also it would be good to include in this comment why it is advantageous to convert zexts in to sexts.  It's not immediately clear because like zext for some values of a and b: sext(a + b) != sext(a) + sext(b).  Include the how sext and add with positive result is handled as a special case while no such case exists for zext.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:312
@@ -296,3 +311,3 @@
                                             BinaryOperator *BO,
                                             bool NonNegative) {
   // We only consider ADD, SUB and OR, because a non-zero constant found in
----------------
Doesn't have to be done in this change, but this function would be easier to follow if every if statement conditionally returned true and then false was returned at the bottom (or vice versa).  That is, the if statements should explicitly enumerate the accepted cases.  As it is, some return true some return false which implies some dependencies between the ifs which makes the logic more complicated than necessary to follow.

================
Comment at: lib/Transforms/Scalar/SeparateConstOffsetFromGEP.cpp:635
@@ +634,3 @@
+        // further have zext(a) <= ObjectSize <= max signed value of typeof(a),
+        // we can prove zext(a) == sext(a).
+        if (ObjectSize <=
----------------
This comment is a little confusing.  Maybe something like:

For GEP operand a, if a <= max signed value of typeof(a), then the sign bit of a is zero and sext(a) = zext(a).  Because the GEP is inbounds, we know a <= ObjectSize, so the necessary condition can be reduced to ObjectSize <= max signed value of typeof(a).



================
Comment at: test/Transforms/SeparateConstOffsetFromGEP/NVPTX/split-gep-and-gvn.ll:102
@@ +101,3 @@
+
+; Similar to @sum_of_array3, but extends array indices using zext instead of
+; sext. e.g., array[zext(x + 1)][zext(y + 1)].
----------------
Similar to sum_of_array2

http://reviews.llvm.org/D4060






More information about the llvm-commits mailing list