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

[hfinkel at anl.gov]

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:480
@@ +479,3 @@
+
+  V = V->stripPointerCasts();
+  Worklist.push_back(V);
----------------
chandlerc wrote:
> Why bother? We strip when popping.
Good point.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:509-510
@@ +508,4 @@
+
+    // If we know how big this object is, and it is less than MaxSize, return
+    // true. Otherwise, return false.
+    if (AllocaInst *AI = dyn_cast<AllocaInst>(P)) {
----------------
chandlerc wrote:
> We don't return true here though, we keep searching. I would state this inverted. "If we find an object that isn't less then MaxSize, we're done, bail." or some such.
Ack. Code is new and already an out-of-sync comment ;)

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:520
@@ +519,3 @@
+      uint64_t TypeSize = DL->getTypeAllocSize(AI->getAllocatedType());
+      if (CS->getZExtValue()*TypeSize > MaxSize)
+        return false;
----------------
chandlerc wrote:
> What if this multiply wraps? I know, I know, it won't but it could in theory. I would write this in some way that no one has to think about that. Maybe just use the APInt we get from the  array size?
Will do.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:541-550
@@ +540,12 @@
+
+// If we're indexing into an object of a known size, and the outer index is
+// not a constant, but having any value but zero would lead to undefined
+// behavior, replace it with zero.
+
+// For example, if we have:
+// @f.a = private unnamed_addr constant [1 x i32] [i32 12], align 4
+// ...
+// %arrayidx = getelementptr inbounds [1 x i32]* @f.a, i64 0, i64 %x
+// ... = load i32* %arrayidx, align 4
+// Then we know that we can replace %x in the GEP with i64 0.
+static bool canReplaceGEPIdxWithZero(InstCombiner &IC, GetElementPtrInst *GEPI,
----------------
chandlerc wrote:
> Merge the comment blocks? Maybe make them doxygen?
> 
> I think it would be nice to generalize this to fold *any* GEP index which would trigger UB if > zero to zero. Maybe just leave a FIXME if doing this is hard? You could maybe put it better down below to not bail at the first non-zero constant index, but to keep going to the first non-constant index.
> 
> This is only sound for inbounds GEPs, no? I would probably restrict it to them, see below.
> Merge the comment blocks? Maybe make them doxygen?

Merged.

> I think it would be nice to generalize this to fold *any* GEP index which would trigger UB if > zero to zero. Maybe just leave a FIXME if doing this is hard? You could maybe put it better down below to not bail at the first non-zero constant index, but to keep going to the first non-constant index.

I'll add a FIXME (for both). I need to think about how to do that. For the second point, yes, although we'd also need to keep track of the offsets those indices implied from the start of the object.

> This is only sound for inbounds GEPs, no? I would probably restrict it to them, see below.

Ah, yes, if there are other indices then, even if positive, they could wrap.


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:554
@@ +553,3 @@
+  const DataLayout *DL = IC.getDataLayout();
+  if (GEPI->getNumOperands() < 2 || !DL)
+    return false;
----------------
chandlerc wrote:
> What GEP has fewer than 2 operands but passes the verifier?
Good question. But SimplifyGEPInst in lib/Analysis/InstructionSimplify.cpp has:

  // getelementptr P -> P.
  if (Ops.size() == 1)
    return Ops[0];

So, to be safe...

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:588-591
@@ +587,6 @@
+
+  // If there are more indices after the one we might replace with a zero, make
+  // sure they're all positive. 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 AllPositive = [&]() -> bool {
----------------
chandlerc wrote:
> You don't need to do this. If the GEP is inbounds, than all intermediate pointer values must be inbounds, so you can skip checking the tail once you have observed badness. However, see my comment below.
> 
> Also, the implementation checks for non-negative numbers, not positive numbers.
> You don't need to do this. If the GEP is inbounds, than all intermediate pointer values must be inbounds, so you can skip checking the tail once you have observed badness. However, see my comment below.

Right, but one past the end still works, and then we could use a negative offset of some contained array to get back inside the object.

> Also, the implementation checks for non-negative numbers, not positive numbers.

Indeed; Positive -> NonNegative

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


================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:827-831
@@ +826,7 @@
+    unsigned Idx;
+    if (canReplaceGEPIdxWithZero(*this, GEPI, &SI, Idx)) {
+      Instruction *NewGEPI = GEPI->clone();
+      NewGEPI->setOperand(Idx,
+        ConstantInt::get(GEPI->getOperand(Idx)->getType(), 0));
+      NewGEPI->insertBefore(GEPI);
+      SI.setOperand(SI.getPointerOperandIndex(), NewGEPI);
----------------
chandlerc wrote:
> Fold this into the helper so that it returns the rewritten GEP?
Sure.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:833
@@ +832,3 @@
+      SI.setOperand(SI.getPointerOperandIndex(), NewGEPI);
+      Worklist.Add(NewGEPI);
+      return &SI;
----------------
chandlerc wrote:
> As long as you use the instruction combiner's IR builder, this isn't necessary.
Right, but I'm not.

http://reviews.llvm.org/D7776

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