[PATCH] D38501: [ValueTracking] Fix a misuse of APInt in GetPointerBaseWithConstantOffset

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: lib/Analysis/ValueTracking.cpp:3068
+      ByteOffset += GEPOffset.sextOrTrunc(ByteOffset.getBitWidth());
 
       Ptr = GEP->getPointerOperand();
----------------
efriedma wrote:
> hfinkel wrote:
> > We still need to make sure that the result doesn't overflow. I think something like this would work:
> > 
> >   APInt OrigByteOffset(ByteOffset);
> > 
> >   ByteOffset += GEPOffset.sextOrTrunc(ByteOffset.getBitWidth());
> >   if (ByteOffset.getActiveBits() >= 64) {
> >     ByteOffset = OrigByteOffset;
> >     break;
> >   }
> > 
> Why does it matter if the result overflows?  GEP arithmetic is wrapping.
> Why does it matter if the result overflows? GEP arithmetic is wrapping.

For large pointers (as noted below). Since you mentioned the assertion, I'll note that its condition is a bit different, and we should match it here:

  APInt OrigByteOffset(ByteOffset);

  ByteOffset += GEPOffset.sextOrTrunc(ByteOffset.getBitWidth());
  if (ByteOffset.getMinSignedBits() > 64) {
    ByteOffset = OrigByteOffset;
    break;
  }


================
Comment at: lib/Analysis/ValueTracking.cpp:3081
   }
   Offset = ByteOffset.getSExtValue();
   return Ptr;
----------------
efriedma wrote:
> Yes, you need a check to make sure the value can be represented in 64 bits.  getSExtValue() will cause an assertion failure if it doesn't.
> 
> Granted, that's only relevant if you have pointers wider than 64 bits.
> Yes, you need a check to make sure the value can be represented in 64 bits. getSExtValue() will cause an assertion failure if it doesn't.

You're right (I had thought it would silently saturate, so the situation is better than I thought).

> Granted, that's only relevant if you have pointers wider than 64 bits.

Yes. Isn't that the point of this patch?


https://reviews.llvm.org/D38501