[PATCH] Remove unnecessary variable indexing into single-element arrays

[Chandler Carruth]

Generally looks fine to commit whenever. Mostly small bits below.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:474
@@ +473,3 @@
+// or equal to the provided maximum size, then return true. Otherwise, return
+// false (constant global values and allocas fall into this category).
+static bool isObjectSizeLessThanOrEq(Value *V, uint64_t MaxSize,
----------------
Add a FIXME to hoist this into Analysis or somewhere it can be re-used? We should make it widely available. This is a great pattern to detect.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:592-595
@@ +591,6 @@
+
+  // If there are more indices after the one we might replace with a zero, make
+  // sure they're all non-negative. If any of them are negative, the overall
+  // address being computed might be before the base address determined by the
+  // first non-zero index.
+  auto AllNonNegative = [&]() -> bool {
----------------
Maybe too clever, but what do you think about bailing the first time you see a positive index as well since you're checking for inbounds? You could make the predicate "canBacktrackFromPastEnd" or some such. As soon as you see a potentially negative index return true, as soon as you see a definitely positive index, return false, if you exhaust the indices return false.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:596
@@ +595,3 @@
+  // first non-zero index.
+  auto AllNonNegative = [&]() -> bool {
+    for (unsigned i = Idx+1, e = GEPI->getNumOperands(); i != e; ++i) {
----------------
nit: no need for explicit return type. You can make it more explicit this is a predicate with an 'is' or can' name prefix...

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:632-634
@@ -491,1 +631,5 @@
 
+  // If we're indexing into an object with a variable index for the memory
+  // access, but the object has only one element, we can assume that the index
+  // will always be zero.
+  if (GetElementPtrInst *GEPI = dyn_cast<GetElementPtrInst>(Op)) {
----------------
hfinkel wrote:
> chandlerc wrote:
> > Gah. Almost. But not quite.
> > 
> > We have to be able to form one-past-the-end of the object. So if all the subsequent pointers are 0, the one that strides the object could be 1 instead of 0.
> > 
> > I think we need to look for a *positive* tail (not just non-negative) of the GEP, or we need to look at the users and fold if they would be UB for a one-past-the-end GEP. Essentially, as long as we're post-dominated or only used by a load or store, we're golden.
> No, I think this is fine. We do need to be able to form one-past-the-end, but we don't need to load from it. Here I know we're loading from it, so it can't be the one-past-the-end pointer.
> 
> This is why I clone the GEP and then change it, so only the load or store gets the rewritten GEP, not any other users of the original pointer.
> 
Totally missed that you were carefully only transforming a load or store's GEP. Yea, we're on the same page now.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:833
@@ +832,3 @@
+      SI.setOperand(SI.getPointerOperandIndex(), NewGEPI);
+      Worklist.Add(NewGEPI);
+      return &SI;
----------------
hfinkel wrote:
> chandlerc wrote:
> > As long as you use the instruction combiner's IR builder, this isn't necessary.
> Right, but I'm not.
Could you? I find that pattern cleaner. We pass IR builders into helper functions regularly.

http://reviews.llvm.org/D7776

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