[PATCH] D16230: isSafeToLoadUnconditionally cleanup

[Artur Pilipenko via llvm-commits]

apilipenko added inline comments.

================
Comment at: lib/Analysis/Loads.cpp:96
@@ -95,3 @@
-    // to determine if they were in fact provided.
-    if (!GV->mayBeOverridden()) {
-      BaseType = GV->getType()->getElementType();
----------------
reames wrote:
> The code in isDereferenceableAndAlignedPointer uses hasExternalWeakLinkage.  Are these different in an interesting way?
This code prohibits any overriding of the global because it allegedly might change its size (I write allegedly here because I don't know how can this happen). The size is important here because later it checks that the access is within the allocation size of the object (see the comment on line 114). 

isDereferenceableAndAlignedPointer cares only about linkage which might result in null globals and don't care about size change because it doesn't use size to determine dereferenceability, but rather does it "symbolically".

================
Comment at: lib/Analysis/Loads.cpp:111
@@ -110,3 @@
-    if (BaseAlign == 0)
-      BaseAlign = DL.getPrefTypeAlignment(BaseType);
-
----------------
reames wrote:
> I'm really not sure this code is dead.  Specifically, this is using getPrefTypeAlignment whereas the code in isAligned, via isDereferenceableAndAlignedPointer, is using getABITypeAlignment.  (Specifically, for the alloca case.)
> 
> For the GV case, we use getPrefTypeAlignment which is documented as being the preferred *stack* alignment vs the global specific one in isAligned.  Again, different.  
> 
> I'm not sure the current code is *correct*, but this definitely doesn't appear to be NFC.  Can you comment on this?
This is NFC for GV. isDereferenceableAndAlignedPointer calls isAligned, which calls getAlignment, which in turn handles GV without specified alignment. For strongly linked globals it's DL.getPrefTypeAlignment. This code only handles strongly linked globals and also uses DL.getPrefTypeAlignment.

For allocas isDereferenceableAndAlignedPointer is more conservative by using getABITypeAlignment. I can't find any confirmation that using preferred alignment is correct here. For example, computeKnownBits assumes that default alignment for an alloca is ABI alignment.

================
Comment at: lib/Analysis/Loads.cpp:114
@@ -113,3 @@
-    if (Align <= BaseAlign) {
-      // Check if the load is within the bounds of the underlying object.
-      if (ByteOffset + LoadSize <= DL.getTypeAllocSize(BaseType) &&
----------------
reames wrote:
> This part does not appear to be handled in the isDereferenceAndAlignedPointer.  Specifically, this is checking *dereferenceability*.  This is likely a bug in isDereferenceAndAlignedPointer.
This check here is required because the base might have a type different from the access type. That can happen because GetPointerBaseWithConstantOffset strips bitcasts.
```
%base.i8 = alloca i8
%base.32 = bitcast %base to i32
%v = load i32, %base.32
```

On the other hand isDereferenceAndAlignedPointer doesn't strip bitcasts from smaller types to larger types.


http://reviews.llvm.org/D16230