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

Jingyue Wu jingyue at google.com
Sun Jun 8 15:03:03 PDT 2014


================
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
----------------
Mark Heffernan wrote:
> 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.
done

================
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
----------------
Mark Heffernan wrote:
> 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.
ack'ed

================
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 <=
----------------
Mark Heffernan wrote:
> 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).
> 
> 
done

================
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)].
----------------
Mark Heffernan wrote:
> Similar to sum_of_array2
actually more similar to sum_of_array

http://reviews.llvm.org/D4060






More information about the llvm-commits mailing list