Take alignment into account in isSafeToLoadUnconditionally

[apilipenko]

Summary

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

[reames]

lib/Analysis/Loads.cpp

69 ↗

(On Diff #27758)

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

137 ↗

(On Diff #27758)

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 ↗

(On Diff #27758)

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

[apilipenko]

lib/Analysis/Loads.cpp

137 ↗

(On Diff #27758)

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 ↗

(On Diff #27758)

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 ↗

(On Diff #27758)

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.

llvm/trunk/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();
+
+  // 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));
+
   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.
 ///

llvm/trunk/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]]