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

[Hal Finkel via llvm-commits]

hfinkel added inline comments.

================
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);
----------------
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.



http://reviews.llvm.org/D15146