[PATCH] D16230: isSafeToLoadUnconditionally cleanup

[Artur Pilipenko via llvm-commits]

apilipenko added inline comments.

================
Comment at: lib/Analysis/Loads.cpp:111
@@ -110,3 @@
-    if (BaseAlign == 0)
-      BaseAlign = DL.getPrefTypeAlignment(BaseType);
-
----------------
apilipenko wrote:
> 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.
I've just found Hal's reply on llvm-dev which confirms that using preferred alignment as default alignment for allocas is correct:
http://lists.llvm.org/pipermail/llvm-dev/2015-June/086277.html

But, changing this in isDereferenceableAndAlignedPointer is a separate improvement.


http://reviews.llvm.org/D16230