[PATCH] D15597: [ValueTracking] Handle opaque types in isDereferenceableAndAlignedPointer.

[Michael Zolotukhin via llvm-commits]

mzolotukhin added a comment.

Hi Philip, Artur, Gerolf,

Thanks for the helpful replies, please find my comments inline.

Michael


================
Comment at: lib/Analysis/ValueTracking.cpp:3304
@@ -3303,2 +3303,3 @@
+        Offset.isNonNegative())
       if (isDereferenceableFromAttribute(BV, Offset, Ty, DL, CtxI, DT, TLI) &&
           isAligned(BV, Offset, Align, DL))
----------------
Gerolf wrote:
> The call to isDeferenceableFromAttribute does not look correct. Should Ty match BV's type. I think the invocation without the Ty parameter is the one needed here. Accidentally this would fix the assertion bug, too, since the subsequent call to isAligned would not happen.
If I'm not mistaken, the invocation without the `Ty` parameter goes without the `Offset` parameter as well, which is required in this case. So, we can't use another version of `isDeferenceableFromAttribute ` here.

That said, the case when BV's type doesn't match V's type is indeed a special case - should we maybe just bailout in this case, or is it expected, and we know how to properly handle it?

================
Comment at: lib/Analysis/ValueTracking.cpp:3305
@@ -3303,3 +3304,3 @@
       if (isDereferenceableFromAttribute(BV, Offset, Ty, DL, CtxI, DT, TLI) &&
           isAligned(BV, Offset, Align, DL))
         return true;
----------------
Gerolf wrote:
> I think the isAligned code is not correct either. The following should be changed to
> 03178   APInt BaseAlign(Offset.getBitWidth(), getAlignment(Base, DL));
> 03179 
> 03180   if (!BaseAlign) {
> 03181     Type *Ty = Base->getType()->getPointerElementType();
> 03182     BaseAlign = DL.getABITypeAlignment(Ty);
> 03183   }
> Type *Ty = Base->getTyoe()->getPointerElementType();
> if (!isSized(Ty)) return false;
> APInt ...
> But again, just to fix the assertion, this is not necessary. A FIXME would do for now.
> 
The problem with fixing it in `isAligned` is that we have two versions of that function:
```
static bool isAligned(const Value *Base, unsigned Align, const DataLayout &DL)
```
and
```
static bool isAligned(const Value *Base, APInt Offset, unsigned Align, const DataLayout &DL)
```
The first calls the second, both of them need the check, and in our case we call directly the second one.
Thus, if we sink `isSized` check to these functions, we'll do this check twice if the first version is called. Considering this, I think the best fix is to check `isSized` before calling `isAligned`, as we do in the current patch. Does it sound right?


http://reviews.llvm.org/D15597