[PATCH] D16230: isSafeToLoadUnconditionally cleanup

[Philip Reames via llvm-commits]

reames requested changes to this revision.
This revision now requires changes to proceed.

================
Comment at: lib/Analysis/Loads.cpp:96
@@ -95,3 @@
-    // to determine if they were in fact provided.
-    if (!GV->mayBeOverridden()) {
-      BaseType = GV->getType()->getElementType();
----------------
The code in isDereferenceableAndAlignedPointer uses hasExternalWeakLinkage.  Are these different in an interesting way?

================
Comment at: lib/Analysis/Loads.cpp:111
@@ -110,3 @@
-    if (BaseAlign == 0)
-      BaseAlign = DL.getPrefTypeAlignment(BaseType);
-
----------------
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?

================
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) &&
----------------
This part does not appear to be handled in the isDereferenceAndAlignedPointer.  Specifically, this is checking *dereferenceability*.  This is likely a bug in isDereferenceAndAlignedPointer.


http://reviews.llvm.org/D16230