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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: llvm/lib/Analysis/InlineCost.cpp:1124
+      else {
+        APInt OffsetWithCorrectSize = BaseAndOffset.second.sext(IntegerSize);
+        std::pair<Value *, APInt> BaseAndOffsetWithCorrectSize =
----------------
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.)


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

https://reviews.llvm.org/D90610