Make FindAvailableLoadedValue TBAA aware

[thopre]

Summary

FindAvailableLoadedValue() relies on FindAvailablePtrLoadStore() to run
the alias analysis when searching for an equivalent value. However,
FindAvailablePtrLoadStore() calls the alias analysis framework with a
memory location for the load constructed from an address and a size,
which thus lacks TBAA metadata info. This commit modifies
FindAvailablePtrLoadStore() to accept an optional memory location as
parameter to allow FindAvailableLoadedValue() to create it based on the
load instruction, which would then have TBAA metadata info attached.

[thopre]

Note: the test added in this commit currently fails because of https://reviews.llvm.org/D99022 (issue reported in https://reviews.llvm.org/D99022#2644973).

[nikic]

llvm/include/llvm/Analysis/Loads.h

168

Why make it optional? The only None use seems to be in JumpThreading, and we should be able to pass a location there as well.

(Could also drop the Ptr argument if it's required, as the memory location includes the pointer.)

[nikic]

Looks fine conceptually. I'd recommend dropping the dependency on the LoopRotate patch and testing this via InstCombine instead, which uses this in a much more direct way.

[jdoerfert]

llvm/include/llvm/Analysis/Loads.h

168

+1, you can always make a MemoryLocation from a pointer anyway.

[lebedev.ri]

This seems like a worthwhile change regardless of LoopRotate.
Can you perhaps add a test for one of the passes that already use FindAvailableLoadedValue()?

[thopre]

• Make patch standalone
• Add InstCombine testcase
• Fix naming of function while modifying parameters
• Make MemoryLocation mandatory

[lebedev.ri]

llvm/test/Transforms/InstCombine/load-no-aliasing.ll

1 ↗

(On Diff #333007)

Please precommit using llvm/utils/update_test_checks.py

[nikic]

llvm/lib/Transforms/Scalar/JumpThreading.cpp

1396

The AATags have already been extracted above (before the loop), so there should be no need to do so again here.

llvm/test/Transforms/InstCombine/load-no-aliasing.ll

1 ↗

(On Diff #333007)

The -evaluate-aa-metadata flag here is not needed -- this is used for directly printing modref information, not for normal transformation tests.

[thopre]

• Remove redundant AAAATags
• Rebase on committed baseline testcase
• Remove -evaluate-aa-metadata

llvm/test/Transforms/InstCombine/load-no-aliasing.ll

1 ↗

(On Diff #333007)

Sorry I only saw that after committing.

[nikic]

LGTM

llvm/include/llvm/Analysis/Loads.h

 
 class AAResults;
 class DataLayout;
 class DominatorTree;
 class Instruction;
 class LoadInst;
 class Loop;
 class MDNode;
+class MemoryLocation;
 class ScalarEvolution;
 class TargetLibraryInfo;
 
 /// Return true if this is always a dereferenceable pointer. If the context
 /// instruction is specified perform context-sensitive analysis and return true
 /// if the pointer is dereferenceable at the specified instruction.
 bool isDereferenceablePointer(const Value *V, Type *Ty,
                               const DataLayout &DL,

 /// is zero, the whole block will be scanned.
 /// \param AA Optional pointer to alias analysis, to make the scan more
 /// precise.
 /// \param [out] IsLoadCSE Whether the returned value is a load from the same
 /// location in memory, as opposed to the value operand of a store.
 ///
 /// \returns The found value, or nullptr if no value is found.
 Value *FindAvailablePtrLoadStore(Value *Ptr, Type *AccessTy, bool AtLeastAtomic,
-                                 BasicBlock *ScanBB,
+                                 Optional<MemoryLocation>, BasicBlock *ScanBB,
                                  BasicBlock::iterator &ScanFrom,
                                  unsigned MaxInstsToScan, AAResults *AA,
                                  bool *IsLoadCSE, unsigned *NumScanedInst);
 
 /// Returns true if a pointer value \p A can be replace with another pointer
 /// value \B if they are deemed equal through some means (e.g. information from
 /// conditions).
 /// NOTE: the current implementations is incomplete and unsound. It does not
 /// reject all invalid cases yet, but will be made stricter in the future. In
 /// particular this means returning true means unknown if replacement is safe.
 bool canReplacePointersIfEqual(Value *A, Value *B, const DataLayout &DL,
                                Instruction *CtxI);
 }
 
 #endif

llvm/lib/Analysis/Loads.cpp

 //===----------------------------------------------------------------------===//
 
 #include "llvm/Analysis/Loads.h"
 #include "llvm/Analysis/AliasAnalysis.h"
 #include "llvm/Analysis/AssumeBundleQueries.h"
 #include "llvm/Analysis/CaptureTracking.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/Analysis/MemoryBuiltins.h"
+#include "llvm/Analysis/MemoryLocation.h"
 #include "llvm/Analysis/ScalarEvolution.h"
 #include "llvm/Analysis/ScalarEvolutionExpressions.h"
 #include "llvm/Analysis/TargetLibraryInfo.h"
 #include "llvm/Analysis/ValueTracking.h"
 #include "llvm/IR/DataLayout.h"
 #include "llvm/IR/GlobalAlias.h"
 #include "llvm/IR/GlobalVariable.h"
 #include "llvm/IR/IntrinsicInst.h"
Value *llvm::FindAvailableLoadedValue(LoadInst *Load,
                                       BasicBlock::iterator &ScanFrom,
                                       unsigned MaxInstsToScan,
                                       AAResults *AA, bool *IsLoad,
                                       unsigned *NumScanedInst) {
   // Don't CSE load that is volatile or anything stronger than unordered.
   if (!Load->isUnordered())
     return nullptr;
 
-  return FindAvailablePtrLoadStore(
-      Load->getPointerOperand(), Load->getType(), Load->isAtomic(), ScanBB,
-      ScanFrom, MaxInstsToScan, AA, IsLoad, NumScanedInst);
+  MemoryLocation Loc = MemoryLocation::get(Load);
+  return FindAvailablePtrLoadStore(Load->getPointerOperand(), Load->getType(),
+                                   Load->isAtomic(), Loc, ScanBB, ScanFrom,
+                                   MaxInstsToScan, AA, IsLoad, NumScanedInst);
 }
 
 // Check if the load and the store have the same base, constant offsets and
 // non-overlapping access ranges.
 static bool AreNonOverlapSameBaseLoadAndStore(
     Value *LoadPtr, Type *LoadTy, Value *StorePtr, Type *StoreTy,
     const DataLayout &DL) {
   APInt LoadOffset(DL.getTypeSizeInBits(LoadPtr->getType()), 0);
if (CastInst::isBitOrNoopPointerCastable(Val->getType(), AccessTy, DL)) {
         *IsLoadCSE = false;
       return Val;
     }
   }
 
   return nullptr;
 }
 
-Value *llvm::FindAvailablePtrLoadStore(Value *Ptr, Type *AccessTy,
-                                       bool AtLeastAtomic, BasicBlock *ScanBB,
-                                       BasicBlock::iterator &ScanFrom,
-                                       unsigned MaxInstsToScan,
-                                       AAResults *AA, bool *IsLoadCSE,
-                                       unsigned *NumScanedInst) {
+Value *llvm::FindAvailablePtrLoadStore(
+    Value *Ptr, Type *AccessTy, bool AtLeastAtomic,
+    Optional<MemoryLocation> Loc, BasicBlock *ScanBB,
+    BasicBlock::iterator &ScanFrom, unsigned MaxInstsToScan, AAResults *AA,
+    bool *IsLoadCSE, unsigned *NumScanedInst) {
   if (MaxInstsToScan == 0)
     MaxInstsToScan = ~0U;
 
   const DataLayout &DL = ScanBB->getModule()->getDataLayout();
   Value *StrippedPtr = Ptr->stripPointerCasts();
 
   while (ScanFrom != ScanBB->begin()) {
     // We must ignore debug info directives when counting (otherwise they
while (ScanFrom != ScanBB->begin()) {
     --ScanFrom;
 
     if (Value *Available = getAvailableLoadStore(Inst, StrippedPtr, AccessTy,
                                                  AtLeastAtomic, DL, IsLoadCSE))
       return Available;
 
     // Try to get the store size for the type.
     auto AccessSize = LocationSize::precise(DL.getTypeStoreSize(AccessTy));
+    if (!Loc)
+      Loc = MemoryLocation(StrippedPtr, AccessSize);
 
     if (StoreInst *SI = dyn_cast<StoreInst>(Inst)) {
       Value *StorePtr = SI->getPointerOperand()->stripPointerCasts();
 
       // If both StrippedPtr and StorePtr reach all the way to an alloca or
       // global and they are different, ignore the store. This is a trivial form
       // of alias analysis that is important for reg2mem'd code.
       if ((isa<AllocaInst>(StrippedPtr) || isa<GlobalVariable>(StrippedPtr)) &&
           (isa<AllocaInst>(StorePtr) || isa<GlobalVariable>(StorePtr)) &&
           StrippedPtr != StorePtr)
         continue;
 
       if (!AA) {
         // When AA isn't available, but if the load and the store have the same
         // base, constant offsets and non-overlapping access ranges, ignore the
         // store. This is a simple form of alias analysis that is used by the
         // inliner. FIXME: use BasicAA if possible.
         if (AreNonOverlapSameBaseLoadAndStore(
                 Ptr, AccessTy, SI->getPointerOperand(),
                 SI->getValueOperand()->getType(), DL))
           continue;
       } else {
         // If we have alias analysis and it says the store won't modify the
         // loaded value, ignore the store.
-        if (!isModSet(AA->getModRefInfo(SI, StrippedPtr, AccessSize)))
+        if (!isModSet(AA->getModRefInfo(SI, *Loc)))
           continue;
       }
 
       // Otherwise the store that may or may not alias the pointer, bail out.
       ++ScanFrom;
       return nullptr;
     }
 
     // If this is some other instruction that may clobber Ptr, bail out.
     if (Inst->mayWriteToMemory()) {
       // If alias analysis claims that it really won't modify the load,
       // ignore it.
-      if (AA && !isModSet(AA->getModRefInfo(Inst, StrippedPtr, AccessSize)))
+      if (AA && !isModSet(AA->getModRefInfo(Inst, *Loc)))
         continue;
 
       // May modify the pointer, bail out.
       ++ScanFrom;
       return nullptr;
     }
   }
 
for (Instruction &Inst : make_range(++Load->getReverseIterator(),
 
     if (Inst.mayWriteToMemory())
       MustNotAliasInsts.push_back(&Inst);
   }
 
   // If we found an available value, ensure that the instructions in between
   // did not modify the memory location.
   if (Available) {
-    auto AccessSize = LocationSize::precise(DL.getTypeStoreSize(AccessTy));
+    MemoryLocation Loc = MemoryLocation::get(Load);
     for (Instruction *Inst : MustNotAliasInsts)
-      if (isModSet(AA.getModRefInfo(Inst, StrippedPtr, AccessSize)))
+      if (isModSet(AA.getModRefInfo(Inst, Loc)))
         return nullptr;
   }
 
   return Available;
 }
 
 bool llvm::canReplacePointersIfEqual(Value *A, Value *B, const DataLayout &DL,
                                      Instruction *CtxI) {

llvm/lib/Transforms/Scalar/JumpThreading.cpp

for (BasicBlock *PredBB : predecessors(LoadBB)) {
     // NOTE: We don't CSE load that is volatile or anything stronger than
     // unordered, that should have been checked when we entered the function.
     assert(LoadI->isUnordered() &&
            "Attempting to CSE volatile or atomic loads");
     // If this is a load on a phi pointer, phi-translate it and search
     // for available load/store to the pointer in predecessors.
     Value *Ptr = LoadedPtr->DoPHITranslation(LoadBB, PredBB);
     PredAvailable = FindAvailablePtrLoadStore(
-        Ptr, LoadI->getType(), LoadI->isAtomic(), PredBB, BBIt,
+        Ptr, LoadI->getType(), LoadI->isAtomic(), None, PredBB, BBIt,
         DefMaxInstsToScan, AA, &IsLoadCSE, &NumScanedInst);
 
     // If PredBB has a single predecessor, continue scanning through the
     // single predecessor.
     BasicBlock *SinglePredBB = PredBB;
     while (!PredAvailable && SinglePredBB && BBIt == SinglePredBB->begin() &&
            NumScanedInst < DefMaxInstsToScan) {
       SinglePredBB = SinglePredBB->getSinglePredecessor();
       if (SinglePredBB) {
         BBIt = SinglePredBB->end();
         PredAvailable = FindAvailablePtrLoadStore(
-            Ptr, LoadI->getType(), LoadI->isAtomic(), SinglePredBB, BBIt,
+            Ptr, LoadI->getType(), LoadI->isAtomic(), None, SinglePredBB, BBIt,
             (DefMaxInstsToScan - NumScanedInst), AA, &IsLoadCSE,
             &NumScanedInst);
       }
     }
 
     if (!PredAvailable) {
       OneUnavailablePred = PredBB;
       continue;

llvm/test/Transforms/LoopRotate/load-hoist.ll

-; RUN: opt -S -loop-rotate < %s -verify-loop-info -verify-dom-info | FileCheck %s
-; RUN: opt -S -loop-rotate < %s -verify-loop-info -verify-dom-info -enable-mssa-loop-dependency=true -verify-memoryssa | FileCheck %s
+; RUN: opt -S -tbaa -evaluate-aa-metadata -loop-rotate < %s -verify-loop-info -verify-dom-info | FileCheck %s
+; RUN: opt -S -tbaa -evaluate-aa-metadata -loop-rotate < %s -verify-loop-info -verify-dom-info -enable-mssa-loop-dependency=true -verify-memoryssa | FileCheck %s
 
 ; Invariant loads are hoisted if no aliasing occurs
 ; CHECK-LABEL: @load(
 ; CHECK-NEXT:  [[ENTRY:.*]]:
 ; CHECK-NEXT:    [[LD:%.*]] = load i32, i32* %src, align 4
 ; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i32 [[LD]], 100
 ; CHECK-NEXT:    br label %[[FOR_BODY:.*]]
 ; CHECK:       [[FOR_BODY]]:
for.body:                                         ; preds = %for.cond
   %inc = add nsw i32 %i.0, 1
   store i32 0, i32* %dst, align 4
   br label %for.cond
 
 for.end:                                          ; preds = %for.cond
   ret i32 %ld
 }
 
+; Invariant loads are hoisted if TBAA determines no aliasing occurs
+; CHECK-LABEL: @tbaa_load(
+; CHECK-NEXT:  [[ENTRY:.*]]:
+; CHECK-NEXT:    [[LD:%.*]] = load i32, i32* %src, align 4, !tbaa !0
+; CHECK-NEXT:    [[CMP2:%.*]] = icmp slt i32 [[LD]], 100
+; CHECK-NEXT:    br label %[[FOR_BODY:.*]]
+; CHECK:       [[FOR_BODY]]:
+; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[INC:%.*]], %[[FOR_BODY]] ]
+; CHECK-NEXT:    [[INC]] = add nsw i32 [[IV]], 1
+; CHECK-NEXT:    store float 0.000000e+00, float* %dst, align 4, !tbaa !6
+; CHECK-NEXT:    [[CMP1:%.*]] = icmp slt i32 [[INC]], 100
+; CHECK-NEXT:    [[CMP:%.*]] = or i1 [[CMP1]], [[CMP2]]
+; CHECK-NEXT:    br i1 [[CMP]], label %[[FOR_BODY]], label %[[FOR_END:.*]]
+; CHECK:       [[FOR_END]]
+; CHECK-NEXT:    [[LD_LCSSA:%.*]] = phi i32 [ [[LD]], %[[FOR_BODY]] ]
+; CHECK-NEXT:    ret i32 [[LD_LCSSA]]
+define i32 @tbaa_load(i32* %src, float* %dst) nounwind ssp {
+entry:
+  br label %for.cond
+
+for.cond:                                         ; preds = %for.body, %entry
+  %i.0 = phi i32 [ 0, %entry ], [ %inc, %for.body ]
+  %ld = load i32, i32* %src, align 4, !tbaa !5
+  %cmp1 = icmp slt i32 %i.0, 100
+  %cmp2 = icmp slt i32 %ld, 100
+  %cmp = or i1 %cmp1, %cmp2
+  br i1 %cmp, label %for.body, label %for.end
+
+for.body:                                         ; preds = %for.cond
+  %inc = add nsw i32 %i.0, 1
+  store float 0.0, float* %dst, align 4, !tbaa !6
+  br label %for.cond
+
+for.end:                                          ; preds = %for.cond
+  ret i32 %ld
+}
+
 ; Atomic loads are not hoisted
 ; CHECK-LABEL: @atomic_load(
 ; CHECK-NEXT:  [[ENTRY:.*]]:
 ; CHECK-NEXT:    [[LD_1:%.*]] = load atomic i32, i32* %src monotonic, align 4
 ; CHECK-NEXT:    [[CMP2_1:%.*]] = icmp slt i32 [[LD_1]], 100
 ; CHECK-NEXT:    br label %[[FOR_BODY:.*]]
 ; CHECK:       [[FOR_BODY]]:
 ; CHECK-NEXT:    [[IV:%.*]] = phi i32 [ 0, %[[ENTRY]] ], [ [[INC:%.*]], %[[FOR_BODY]] ]

 for.body:                                         ; preds = %for.cond
   %inc = add nsw i32 %i.0, 1
   call void asm sideeffect "", ""()
   br label %for.cond
 
 for.end:                                          ; preds = %for.cond
   ret i32 %ld
 }
+
+!0 = !{!"i32 pointer", !1, i64 0, !4, i64 8}
+!1 = !{!"any pointer", !2, i64 0}
+!2 = !{!"omnipotent char", !3, i64 0}
+!3 = !{!"Simple C++ TBAA"}
+!4 = !{!"int", !1, i64 0}
+!5 = !{!0, !1, i64 0}
+!6 = !{!7, !7, i64 0}
+!7 = !{!"float", !2, i64 0}