[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 inline comments.

================
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
----------------
jmolloy wrote:
> Can't we insert the GEP after the transformed instruction instead of before the PHI?
Theoretically if you have

p0 = phi(x0, x1, ..)
p1 = phi(p0, ..)

you could insert GEPs before x0, x1, etc, and get:
BB1:
  y0 = gep(base, x0)
BB2:
  y1 = gep(base x1)
BB3:
  p0 = phi(y0, y1)
  p1 = phi(p0, ..)

However you end up inserting a lot of geps using this strategy. It's not clear to me that this is a good thing.
So we could also handle this case, but I don't think it's worth it.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:731
@@ +730,3 @@
+
+  DenseMap<Value *, Value *> NewInsts;
+  NewInsts[Base] = ConstantInt::getNullValue(IndexType);
----------------
jmolloy wrote:
> ValueToValueMapTy might help here.
I could use it, but I'm not sure how it would help.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:920
@@ +919,3 @@
+  //   OFFSET1 cmp OFFSET2
+  Value *NewRHS = rewriteAsOffsetGEP(RHS, PtrBase, DL, Nodes);
+  return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Index, NewRHS);
----------------
hfinkel wrote:
> For the particular ICmpInst you've creating here, is this `((gep Ptr, OFFSET1) cmp (gep Ptr, OFFSET2)` or `OFFSET1 cmp OFFSET2`? Given that rewriteAsOffsetGEP returns a GEP that uses PtrBase as its base address, it looks like for former. I see no reason it should not be the latter, however.
> 
> I think this would be clearer if you used the same names in your comments as you did names for the local variables.
> 
Hi Hal,

Should be the latter (in rewriteAsOffsetGEP NewInsts maps the geps to offsets). Maybe rewriteGEPAsOffset would be a better name for that function.

Thanks,
Silviu


http://reviews.llvm.org/D15146