[PATCH] When canonicalizing gep indices, prefer zext when possible

[Philip Reames]

Responded to small comments inline.  I'm not going to both updating the patch just yet.  The high level questions I asked in the original review need to be addressed first.  Rephrased:

Is canonicalizing sign extension to zero extension a good idea on all targets?  It appears to work out well on x86, but do other targets have concerns?  Does this need to be behind a target hook?


================
Comment at: lib/Transforms/InstCombine/InstCombineCasts.cpp:1067-1074
@@ -1066,1 +1066,10 @@
 
+  // If we know that the value being extended is positive, we can use a zext
+  // instead. 
+  bool KnownZero, KnownOne;
+  ComputeSignBit(Src, KnownZero, KnownOne, 0, &CI);
+  if (KnownZero) {
+    Value *ZExt = Builder->CreateZExt(Src, DestTy);
+    return ReplaceInstUsesWith(CI, ZExt);
+  }
+
----------------
majnemer wrote:
> This looks like it can be a separate change.
It could yes, but the broader question of whether this transform is a good idea is global to all the locations.  

When submitting, I'm happy to split this into a separate patch with it's own test.  

================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:1326-1331
@@ -1325,5 +1325,8 @@
         // If we are using a wider index than needed for this platform, shrink
-        // it to what we need.  If narrower, sign-extend it to what we need.
+        // it to what we need.  If narrower, sign-extend it to what we need
+        // (unless we can tell it's positive, in which case zero-extend.)
         // This explicit cast can make subsequent optimizations more obvious.
-        *I = Builder->CreateIntCast(*I, IntPtrTy, true);
+        bool KnownZero, KnownOne;
+        ComputeSignBit(*I, KnownZero, KnownOne, 0, &GEP);
+        *I = Builder->CreateIntCast(*I, IntPtrTy, !KnownZero);
         MadeChange = true;
----------------
majnemer wrote:
> Isn't this change redundant with the one in InstCombineCasts?  It would be nice to have this logic in one place.
Arguably, it is.  I'm fine removing this.

http://reviews.llvm.org/D7255

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/