[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:679
@@ +678,3 @@
+  for (Value *Val : Explored) {
+    for (auto Use = Val->use_begin(); Use != Val->use_end(); ++Use) {
+
----------------
jmolloy wrote:
>    for (auto &Use : Val->uses())  ?
This is strange, Value seems to not have a uses() method.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:805
@@ +804,3 @@
+
+  // If required, create a inttoptr instruction.
+  Value *NewBase = Base;
----------------
jmolloy wrote:
> Isn't this a ptrtoint?
The implementation creates a ptrtoint or inttoptr depending on the second argument (Start-getType() here). We know that Start is a pointer since it is produced by a GEP, so this will create a inttoptr.

================
Comment at: lib/Transforms/InstCombine/InstCombineCompares.cpp:905
@@ +904,3 @@
+  //   OFFSET1 cmp OFFSET2
+  Value *NewRHS = RewriteGEPAsOffset(RHS, PtrBase, DL, Nodes);
+  return new ICmpInst(ICmpInst::getSignedPredicate(Cond), Index, NewRHS);
----------------
hfinkel wrote:
> sbaranga wrote:
> > hfinkel wrote:
> > > sbaranga wrote:
> > > > hfinkel wrote:
> > > > > Still don't find this clear. It seems to me that:
> > > > > 
> > > > >   std::tie(PtrBase, Index) = GetAsConstantIndexedAddress(GEPLHS, DL);
> > > > > 
> > > > > returns PtrBase and Index such that GEP(PtrBase,  Index) == GEPLHS. And then:
> > > > > 
> > > > >   RewriteGEPAsOffset(RHS, PtrBase)
> > > > > 
> > > > > returns GEP(PtrBase, Something) == RHS. And then we compare:
> > > > > 
> > > > >   Index icmp GEP(PtrBase, Something)
> > > > > 
> > > > > which must be wrong. What am I missing?
> > > > > 
> > > > RewriteGEPAsOffset returns the offset, not the GEP.
> > > Ah, I understand now. The GEP it builds is not returned, but is for use by other users of the original GEP being replaced.
> > Yes, that is correct. Would there be any better way to express this?
> Probably not. The code in RewriteGEPAsOffset does say that the GEP is being generated for external users. I did not read it with sufficient care.
> 
> I think however, it is worth nothing in a comment here that RHS is being replaced by a rewritten GEP and the GEP's Index operand it being returned. Otherwise, the fact that RHS is no longer valid after the function call is not obvious.
> 
Thanks, I'll add a comment.


http://reviews.llvm.org/D15146