[PATCH] D90610: [Inline] Fix in handling of ptrtoint in InlineCost

[Mikael Holmén via Phabricator via llvm-commits]

uabelho added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1124
+      else {
+        APInt OffsetWithCorrectSize = BaseAndOffset.second.sext(IntegerSize);
+        std::pair<Value *, APInt> BaseAndOffsetWithCorrectSize =
----------------
spatel wrote:
> uabelho wrote:
> > bjope wrote:
> > > uabelho wrote:
> > > > spatel wrote:
> > > > > bjope wrote:
> > > > > > spatel wrote:
> > > > > > > uabelho wrote:
> > > > > > > > I don't know if sext is what we want to do here, of it that may lead to problems?
> > > > > > > Signed math is what I would expect based on:
> > > > > > > http://llvm.org/docs/LangRef.html#getelementptr-instruction
> > > > > > > "These integers are treated as signed values where relevant."
> > > > > > > ...and we do similar in GEPOperator::accumulateConstantOffset() for example.
> > > > > > > 
> > > > > > Not sure exactly how these ConstantOffsetPtrs are used, but doesn't it matter that ptrtoint is defined as doing a zext if the integer size is greater than the size of the pointer (base+offset)?
> > > > > Hmm...yes, I think you're right:
> > > > > http://llvm.org/docs/LangRef.html#ptrtoint-to-instruction
> > > > > So disregard my GEP comment. It would be great to manufacture an example of this behavior, but I'm not sure how to do that.
> > > > Ok, so I changed to use zext.
> > > > 
> > > > I tried fiddling with testcases to try to find cases where I could see a difference between using sext and zext but I've failed.
> > > Just to clarify, I'm not sure zext is any better (or more correct). My point was that if you have `%x = ptrtoint %y` an `%y` is `%base + %offset`, then `zext(%base+%offset)` isn't neccessarily the same as `zext(%base) + zext(%offset)` (neither the same as `sext(%base) + sext(%offset)`). Unless maybe if these base&offset pairs are guaranteed to be inbound or non-wrapping/non-overflowing or something? Maybe that is implied?
> > I have no idea myself. As I said I tried writing a couple of different variants of the added tests to see if sext/zext/changing at line 1104 made any difference, and I didn't manage to do that.
> > 
> > Changing "<=" to "==" at 1104 solves the problem too and I absolutely don't mind doing that if you all agree that is the best/safest fix.
> > 
> > 
> > 
> > 
> We should proceed with that simplest fix ("==") to avoid crashing. Perf improvement can be a follow-up. IIUC, there is no danger of regressions with that change because it can't succeed currently.
> 
> (Note that D90708 clarified LangRef pointer math semantics, so that might help answer the questions about enhancing this to work correctly with different-sized offsets.)
Great, I don't mind this at all so I just uploaded such a fix.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D90610/new/

https://reviews.llvm.org/D90610