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

[hfinkel at anl.gov]

================
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,
----------------
chandlerc wrote:
> 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.
Sure.

================
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 {
----------------
chandlerc wrote:
> 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.
I'm not sure that works. Even after a positive index, we could have a "larger" negative index, and we could still keep everthing inbounds.

================
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) {
----------------
chandlerc wrote:
> nit: no need for explicit return type. You can make it more explicit this is a predicate with an 'is' or can' name prefix...
Sure.

================
Comment at: lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp:833
@@ +832,3 @@
+      SI.setOperand(SI.getPointerOperandIndex(), NewGEPI);
+      Worklist.Add(NewGEPI);
+      return &SI;
----------------
chandlerc wrote:
> 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.
Could? Yes. But I'm not sure that's better (if I use the builder interface, I still need to worry about subsequently copying over the inbounds flag -- but only if the builder did not fold the GEP to something else anyway, in which case I need to be careful that it is not folded to some other GEP that used to be related to the original GEP's first operand, etc.).

http://reviews.llvm.org/D7776

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