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

[silviu.baranga@arm.com via llvm-commits]

sbaranga added a comment.

Hi James,

Thanks for having a look. I've added some replies inline. I should have a new version of this patch that addresses your comments out soon.

Thanks,
Silviu


================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:604
@@ +603,3 @@
+                                  SmallPtrSetImpl<Value *> &Explored) {
+  SmallVector<Value *, 100> WorkList(1, Start);
+
----------------
jmolloy wrote:
> Do you really expect 100 items in the worklist?
Not frequently, but it is possible. Probably the limit wouldn't be ever reached in practice, but we still need to have it. In my experience if we can't do the transformation we usually bail out early, so it shouldn't be a very big problem.

I can do compile time measurements if you think this would be a problem.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:624
@@ +623,3 @@
+
+      if (Explored.insert(CI->getOperand(0)).second) {
+        WorkList.push_back(CI->getOperand(0));
----------------
jmolloy wrote:
> 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.
My intention was to limit the memory usage of the work list to |V| (you would get |E| if you checked when removing from the worklist). However, it doesn't seem that bad either way.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:761
@@ +760,3 @@
+
+      // Create a new instruction for R.
+      if (auto *CI = dyn_cast<CastInst>(R)) {
----------------
jmolloy wrote:
> 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.
Good idea, thanks! It would technically need to be the reverse order, but that shouldn't be a problem.


http://reviews.llvm.org/D15146