Take alignment into account in isSafeToLoadUnconditionally

[apilipenko]

Summary

Take alignment into account in isSafeToLoadUnconditionally. Separated from the D9791.

[reames]

lib/Analysis/Loads.cpp

69

Please clarify comment as to *why* we want this.

137

From here on looks like a separate change. Please separate. Each logical change should be reviewed independently. Also, you should have a test case for each.

150

Why should the native alignment of the pointee type influence anything? We can freely bitcast pointers before loading.

[apilipenko]

lib/Analysis/Loads.cpp

137

Logically it's a single change to take alignment into account in this function. Actually from here on the main part of the change begins. I see no reason to split it more. Test case is also provided.

150

If we end up here with zero AccessedAlign it means that there is load or store instruction before without explicit alignment specification. From docs about load/stores:
"A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target."
"Overestimating the alignment results in undefined behavior."
So, I assume here that if previous load succeed that pointer alignment is at least ABI alignment.

[apilipenko]

lib/Analysis/Loads.cpp

69

That is a part of load specification:
"A value of 0 or an omitted align argument means that the operation has the ABI alignment for the target."

[hfinkel]

LGTM.

lib/Analysis/Loads.cpp

 /// a quick local scan of the basic block containing \c ScanFrom, to determine
 /// if the address is already accessed.
 ///
 /// This uses the pointee type to determine how many bytes need to be safe to
 /// load from the pointer.
 bool llvm::isSafeToLoadUnconditionally(Value *V, Instruction *ScanFrom,
                                        unsigned Align) {
   const DataLayout &DL = ScanFrom->getModule()->getDataLayout();
+
+  // Require ABI alignment for loads without alignment specification
+  if (Align == 0)
+    Align = DL.getABITypeAlignment(V->getType()->getPointerElementType());
+  assert(isPowerOf2_32(Align));
+
   int64_t ByteOffset = 0;
   Value *Base = V;
   Base = GetPointerBaseWithConstantOffset(V, ByteOffset, DL);
 
   if (ByteOffset < 0) // out of bounds
     return false;
 
   Type *BaseType = nullptr;
bool llvm::isSafeToLoadUnconditionally(Value *V, Instruction *ScanFrom,
   // 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) &&
-          (Align == 0 || (ByteOffset % Align) == 0))
+          ((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
while (BBI != E) {
 
     // If we see a free or a call which may write to memory (i.e. which might do
     // a free) the pointer could be marked invalid.
     if (isa<CallInst>(BBI) && BBI->mayWriteToMemory() &&
         !isa<DbgInfoIntrinsic>(BBI))
       return false;
 
     Value *AccessedPtr;
-    if (LoadInst *LI = dyn_cast<LoadInst>(BBI))
+    unsigned AccessedAlign;
+    if (LoadInst *LI = dyn_cast<LoadInst>(BBI)) {
       AccessedPtr = LI->getPointerOperand();
-    else if (StoreInst *SI = dyn_cast<StoreInst>(BBI))
+      AccessedAlign = LI->getAlignment();
+    } else if (StoreInst *SI = dyn_cast<StoreInst>(BBI)) {
       AccessedPtr = SI->getPointerOperand();
-    else
+      AccessedAlign = SI->getAlignment();
+    } else
+      continue;
+
+    Type *AccessedTy = AccessedPtr->getType()->getPointerElementType();
+    if (AccessedAlign == 0)
+      AccessedAlign = DL.getABITypeAlignment(AccessedTy);
+    if (AccessedAlign < Align)
       continue;
 
     // Handle trivial cases.
     if (AccessedPtr == V)
       return true;
 
-    auto *AccessedTy = cast<PointerType>(AccessedPtr->getType());
     if (AreEquivalentAddressValues(AccessedPtr->stripPointerCasts(), V) &&
-        LoadSize <= DL.getTypeStoreSize(AccessedTy->getElementType()))
+        LoadSize <= DL.getTypeStoreSize(AccessedTy))
       return true;
   }
   return false;
 }
 
 /// \brief Scan the ScanBB block backwards to see if we have the value at the
 /// memory address *Ptr locally available within a small number of instructions.
 ///

test/Transforms/InstCombine/select.ll

entry:
   store i32 0, i32* %y
   ; Block forwarding by storing to %z which could alias either %x or %y.
   store i32 42, i32* %z
   %p = select i1 %flag, i32* %x, i32* %y
   %v = load i32, i32* %p
   ret i32 %v
 }
 
+define i32 @test78_neg(i1 %flag, i32* %x, i32* %y, i32* %z) {
+; The same as @test78 but we can't speculate the load because it can trap
+; if under-aligned.
+; CHECK-LABEL: @test78_neg(
+; CHECK: %p = select i1 %flag, i32* %x, i32* %y
+; CHECK-NEXT: %v = load i32, i32* %p, align 16
+; CHECK-NEXT: ret i32 %v
+entry:
+  store i32 0, i32* %x
+  store i32 0, i32* %y
+  ; Block forwarding by storing to %z which could alias either %x or %y.
+  store i32 42, i32* %z
+  %p = select i1 %flag, i32* %x, i32* %y
+  %v = load i32, i32* %p, align 16
+  ret i32 %v
+}
+
 define float @test79(i1 %flag, float* %x, i32* %y, i32* %z) {
 ; Test that we can speculate the loads around the select even when we can't
 ; fold the load completely away.
 ; CHECK-LABEL: @test79(
 ; CHECK:         %[[V1:.*]] = load float, float* %x
 ; CHECK-NEXT:    %[[V2:.*]] = load float, float* %y
 ; CHECK-NEXT:    %[[S:.*]] = select i1 %flag, float %[[V1]], float %[[V2]]
 ; CHECK-NEXT:    ret float %[[S]]