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

[Chandler Carruth]

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

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

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:520
@@ +519,3 @@
+      uint64_t TypeSize = DL->getTypeAllocSize(AI->getAllocatedType());
+      if (CS->getZExtValue()*TypeSize > MaxSize)
+        return false;
----------------
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?

================
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,
----------------
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.

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

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

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

================
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);
----------------
Fold this into the helper so that it returns the rewritten GEP?

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

http://reviews.llvm.org/D7776

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