isSafeToLoadUnconditionally cleanup

[apilipenko]

Summary

GlobalVariable/AllocaInst cases are handled by isDerferenceableAndAlignedPointer function.

[reames]

lib/Analysis/Loads.cpp

96

The code in isDereferenceableAndAlignedPointer uses hasExternalWeakLinkage. Are these different in an interesting way?

111

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?

114

This part does not appear to be handled in the isDereferenceAndAlignedPointer. Specifically, this is checking *dereferenceability*. This is likely a bug in isDereferenceAndAlignedPointer.

[apilipenko]

lib/Analysis/Loads.cpp

96

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".

111

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.

114

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.

[apilipenko]

lib/Analysis/Loads.cpp

111

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.

[apilipenko]

Ping.

[reames]

Artur, I'm sorry, but I don't feel comfortable signing off on this patch. I'm not convinced that this change actually is NFC and I don't understand this area of code well enough to approving changes. I'm going to have to defer to Hal or another reviewer.

lib/Analysis/Loads.cpp

bool llvm::isSafeToLoadUnconditionally(Value *V, unsigned Align,
   // Zero alignment means that the load has the ABI alignment for the target
   if (Align == 0)
     Align = DL.getABITypeAlignment(V->getType()->getPointerElementType());
   assert(isPowerOf2_32(Align));
 
   if (isDereferenceableAndAlignedPointer(V, Align, DL, ScanFrom, DT, TLI))
     return true;
 
-  int64_t ByteOffset = 0;
-  Value *Base = V;
-  Base = GetPointerBaseWithConstantOffset(V, ByteOffset, DL);
-
-  if (ByteOffset < 0) // out of bounds
-    return false;
-
-  Type *BaseType = nullptr;
-  unsigned BaseAlign = 0;
-  if (const AllocaInst *AI = dyn_cast<AllocaInst>(Base)) {
-    // An alloca is safe to load from as load as it is suitably aligned.
-    BaseType = AI->getAllocatedType();
-    BaseAlign = AI->getAlignment();
-  } else if (const GlobalVariable *GV = dyn_cast<GlobalVariable>(Base)) {
-    // Global variables are not necessarily safe to load from if they are
-    // overridden. Their size may change or they may be weak and require a test
-    // to determine if they were in fact provided.
-    if (!GV->mayBeOverridden()) {
-      BaseType = GV->getType()->getElementType();
-      BaseAlign = GV->getAlignment();
-    }
-  }
-
   PointerType *AddrTy = cast<PointerType>(V->getType());
   uint64_t LoadSize = DL.getTypeStoreSize(AddrTy->getElementType());
 
-  // If we found a base allocated type from either an alloca or global variable,
-  // try to see if we are definitively within the allocated region. We need to
-  // know the size of the base type and the loaded type to do anything in this
-  // case.
-  if (BaseType && BaseType->isSized()) {
-    if (BaseAlign == 0)
-      BaseAlign = DL.getPrefTypeAlignment(BaseType);
-
-    if (Align <= BaseAlign) {
-      // Check if the load is within the bounds of the underlying object.
-      if (ByteOffset + LoadSize <= DL.getTypeAllocSize(BaseType) &&
-          ((ByteOffset % Align) == 0))
-        return true;
-    }
-  }
-
   // Otherwise, be a little bit aggressive by scanning the local block where we
   // want to check to see if the pointer is already being loaded or stored
   // from/to.  If so, the previous load or store would have already trapped,
   // so there is no harm doing an extra load (also, CSE will later eliminate
   // the load entirely).
   BasicBlock::iterator BBI = ScanFrom->getIterator(),
                        E = ScanFrom->getParent()->begin();