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

[Gerolf Hoflehner via llvm-commits]

Gerolf added a subscriber: Gerolf.
Gerolf added a comment.

Caveat: I'm not familiar with this code either. It would be great if someone could double check removing the Ty parameter from  isDeferenceableFromAttribute does not cause problems. But I would be really surprised if Ty didn't have to match BV's type....


================
Comment at: lib/Analysis/ValueTracking.cpp:3296
@@ -3295,3 +3295,3 @@
   if (Align == 0)
     Align = DL.getABITypeAlignment(Ty);
 
----------------
if Ty were opaque here wouldn't there be an assertion in that call already? I guess I don't see why a non-opaque type is guaranteed here. The safe fix would be to check for isSized(Ty) here as well. Since this is orthogonal to the current assert a FIXME should do for now.

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

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



http://reviews.llvm.org/D15597