[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 reviewer: jmolloy.
jmolloy added a comment.
This revision now requires changes to proceed.

Hi Silviu,

I've reviewed the first half of the patch and have a bunch of comments.

Cheers,

James


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:604
@@ +603,3 @@
+                                  SmallPtrSetImpl<Value *> &Explored) {
+  SmallVector<Value *, 100> WorkList(1, Start);
+
----------------
Do you really expect 100 items in the worklist?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:614
@@ +613,3 @@
+
+    Value *V = WorkList.pop_back_val();
+
----------------
Why aren't you inserting V into Explored here, and just incrementing a counter instead of checking Explored.size() above?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:624
@@ +623,3 @@
+
+      if (Explored.insert(CI->getOperand(0)).second) {
+        WorkList.push_back(CI->getOperand(0));
----------------
It looks like you're checking the *next item to be inserted into the worklist* here; I think it'd be easier to just always insert into the worklist and check each item as it's removed from the worklist up above, like GetUnderlyingObjects does.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:630
@@ +629,3 @@
+
+    if (auto *CI = dyn_cast<PtrToIntInst>(V)) {
+      // Make sure this operation doesn't have an implicit zext/sext/trunc.
----------------
This and the previous block can be merged together.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:644
@@ +643,3 @@
+      // future.
+      if (GEP->getNumIndices() != 1)
+        return nullptr;
----------------
These ifs can be merged.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:654
@@ +653,3 @@
+
+    // Scan the operands to see if they are either phi nodes or are equal to
+    // the value.
----------------
Don't understand this comment?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:667
@@ +666,3 @@
+
+  // Make sure that we can do this. Since we can't insert GEPs in a basic
+  // block before a PHI node, we can't easily do this transformation if
----------------
Can't we insert the GEP after the transformed instruction instead of before the PHI?

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:691
@@ +690,3 @@
+
+/// Sets the insertion point of the builder depending on the type of value that
+/// that will constitute an input for the instructions/values that we're going
----------------
I can't parse this comment :(

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:701
@@ +700,3 @@
+  if (auto *I = dyn_cast<Instruction>(V)) {
+    if (!Before)
+      I = &*(++BasicBlock::iterator(I));
----------------
This would be better written as 

  I = &*std::next(I->getIterator());

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:712
@@ +711,3 @@
+  }
+  // Otherwise, this is a constant and we don't need to set a new
+  // insertion point.
----------------
We should assert this.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:731
@@ +730,3 @@
+
+  DenseMap<Value *, Value *> NewInsts;
+  NewInsts[Base] = ConstantInt::getNullValue(IndexType);
----------------
ValueToValueMapTy might help here.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:761
@@ +760,3 @@
+
+      // Create a new instruction for R.
+      if (auto *CI = dyn_cast<CastInst>(R)) {
----------------
It seems a bit strange to do all this work again. You've already explored before; if you'd have just kept the explore order you'd be able to iterate over Explored here without creating it afresh.


http://reviews.llvm.org/D15146