[PATCH] D15146: [InstCombine] Look through PHIs, GEPs, IntToPtrs and PtrToInts to expose more constants when comparing GEPs

[James Molloy via llvm-commits]

jmolloy requested changes to this revision.
jmolloy added a comment.
This revision now requires changes to proceed.

> Please try to name your functions like transformToIndexedCompare instead of TransformToIndexedCompare.


Please do update your function names to begin with a lower case letter as David asked.


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:605
@@ +604,3 @@
+                                  SetVector<Value *> &Explored) {
+  SmallVector<Value *, 100> WorkList(1, Start);
+  Explored.insert(Base);
----------------
OK, but the smallvector value should be optimized for the frequent case. Something like 16 would be more appropriate?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:631
@@ +630,3 @@
+      // We've found some value that we can't explore which is different from
+      // the base. Therefore we can't do this transformation.
+      if (!isa<IntToPtrInst>(V) && !isa<PtrToIntInst>(V) &&
----------------
You should move the comment inside of the if, so it makes sense.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:679
@@ +678,3 @@
+  for (Value *Val : Explored) {
+    for (auto Use = Val->use_begin(); Use != Val->use_end(); ++Use) {
+
----------------
   for (auto &Use : Val->uses())  ?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:805
@@ +804,3 @@
+
+  // If required, create a inttoptr instruction.
+  Value *NewBase = Base;
----------------
Isn't this a ptrtoint?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:851
@@ +850,3 @@
+    if (GEPOperator *GEP = dyn_cast<GEPOperator>(V)) {
+      // We accept only inbouds GEPs here to exclude the possibily of
+      // overflow.
----------------
possibility


http://reviews.llvm.org/D15146