[llvm] 1dbcaf2 - [LAA] Check if dependencies access loop-varying underlying objects.
Florian Hahn via llvm-commits
llvm-commits at lists.llvm.org
Wed Nov 15 13:59:33 PST 2023
Author: Florian Hahn
Date: 2023-11-15T21:58:57Z
New Revision: 1dbcaf277714331bfc60dc6ead31b0e3a300de24
URL: https://github.com/llvm/llvm-project/commit/1dbcaf277714331bfc60dc6ead31b0e3a300de24
DIFF: https://github.com/llvm/llvm-project/commit/1dbcaf277714331bfc60dc6ead31b0e3a300de24.diff
LOG: [LAA] Check if dependencies access loop-varying underlying objects.
This patch adds a new dependence kind UnsafeIndirect, for cases where at
least one of the memory access instructions may access a loop varying object,
e.g. the address of underlying object is loaded inside the loop, like A[B[i]].
We cannot determine direction or distance in those cases, and also are unable
to generate any runtime checks.
This fixes a miscompile, if we attempt to generate runtime checks for
unknown dependencies.
Note that in most cases we do not attempt to generate runtime checks for
unknown dependences, except if FoundNonConstantDistanceDependence is
true.
Fixes https://github.com/llvm/llvm-project/issues/69744.
Added:
Modified:
llvm/include/llvm/Analysis/LoopAccessAnalysis.h
llvm/lib/Analysis/LoopAccessAnalysis.cpp
llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll
Removed:
################################################################################
diff --git a/llvm/include/llvm/Analysis/LoopAccessAnalysis.h b/llvm/include/llvm/Analysis/LoopAccessAnalysis.h
index 3dc7601b9225c07..e39c371b41ec5c1 100644
--- a/llvm/include/llvm/Analysis/LoopAccessAnalysis.h
+++ b/llvm/include/llvm/Analysis/LoopAccessAnalysis.h
@@ -118,6 +118,12 @@ class MemoryDepChecker {
NoDep,
// We couldn't determine the direction or the distance.
Unknown,
+ // At least one of the memory access instructions may access a loop
+ // varying object, e.g. the address of underlying object is loaded inside
+ // the loop, like A[B[i]]. We cannot determine direction or distance in
+ // those cases, and also are unable to generate any runtime checks.
+ IndirectUnsafe,
+
// Lexically forward.
//
// FIXME: If we only have loop-independent forward dependences (e.g. a
@@ -190,7 +196,9 @@ class MemoryDepChecker {
///
/// Only checks sets with elements in \p CheckDeps.
bool areDepsSafe(DepCandidates &AccessSets, MemAccessInfoList &CheckDeps,
- const DenseMap<Value *, const SCEV *> &Strides);
+ const DenseMap<Value *, const SCEV *> &Strides,
+ const DenseMap<Value *, SmallVector<const Value *, 16>>
+ &UnderlyingObjects);
/// No memory dependence was encountered that would inhibit
/// vectorization.
@@ -318,9 +326,11 @@ class MemoryDepChecker {
/// element access it records this distance in \p MinDepDistBytes (if this
/// distance is smaller than any other distance encountered so far).
/// Otherwise, this function returns true signaling a possible dependence.
- Dependence::DepType isDependent(const MemAccessInfo &A, unsigned AIdx,
- const MemAccessInfo &B, unsigned BIdx,
- const DenseMap<Value *, const SCEV *> &Strides);
+ Dependence::DepType
+ isDependent(const MemAccessInfo &A, unsigned AIdx, const MemAccessInfo &B,
+ unsigned BIdx, const DenseMap<Value *, const SCEV *> &Strides,
+ const DenseMap<Value *, SmallVector<const Value *, 16>>
+ &UnderlyingObjects);
/// Check whether the data dependence could prevent store-load
/// forwarding.
diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
index 3d1edd5f038a25e..6803960585d07ea 100644
--- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp
+++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp
@@ -717,6 +717,11 @@ class AccessAnalysis {
MemAccessInfoList &getDependenciesToCheck() { return CheckDeps; }
+ const DenseMap<Value *, SmallVector<const Value *, 16>> &
+ getUnderlyingObjects() {
+ return UnderlyingObjects;
+ }
+
private:
typedef MapVector<MemAccessInfo, SmallSetVector<Type *, 1>> PtrAccessMap;
@@ -762,6 +767,8 @@ class AccessAnalysis {
/// The SCEV predicate containing all the SCEV-related assumptions.
PredicatedScalarEvolution &PSE;
+
+ DenseMap<Value *, SmallVector<const Value *, 16>> UnderlyingObjects;
};
} // end anonymous namespace
@@ -1116,7 +1123,6 @@ bool AccessAnalysis::canCheckPtrAtRT(RuntimePointerChecking &RtCheck,
for (const auto &A : AS) {
Value *Ptr = A.getValue();
bool IsWrite = Accesses.count(MemAccessInfo(Ptr, true));
-
if (IsWrite)
++NumWritePtrChecks;
else
@@ -1331,10 +1337,12 @@ void AccessAnalysis::processMemAccesses() {
typedef SmallVector<const Value *, 16> ValueVector;
ValueVector TempObjects;
- getUnderlyingObjects(Ptr, TempObjects, LI);
+ UnderlyingObjects[Ptr] = {};
+ SmallVector<const Value *, 16> &UOs = UnderlyingObjects[Ptr];
+ ::getUnderlyingObjects(Ptr, UOs, LI);
LLVM_DEBUG(dbgs()
<< "Underlying objects for pointer " << *Ptr << "\n");
- for (const Value *UnderlyingObj : TempObjects) {
+ for (const Value *UnderlyingObj : UOs) {
// nullptr never alias, don't join sets for pointer that have "null"
// in their UnderlyingObjects list.
if (isa<ConstantPointerNull>(UnderlyingObj) &&
@@ -1662,6 +1670,7 @@ MemoryDepChecker::Dependence::isSafeForVectorization(DepType Type) {
case ForwardButPreventsForwarding:
case Backward:
case BackwardVectorizableButPreventsForwarding:
+ case IndirectUnsafe:
return VectorizationSafetyStatus::Unsafe;
}
llvm_unreachable("unexpected DepType!");
@@ -1673,6 +1682,7 @@ bool MemoryDepChecker::Dependence::isBackward() const {
case Forward:
case ForwardButPreventsForwarding:
case Unknown:
+ case IndirectUnsafe:
return false;
case BackwardVectorizable:
@@ -1698,6 +1708,7 @@ bool MemoryDepChecker::Dependence::isForward() const {
case BackwardVectorizable:
case Backward:
case BackwardVectorizableButPreventsForwarding:
+ case IndirectUnsafe:
return false;
}
llvm_unreachable("unexpected DepType!");
@@ -1855,10 +1866,21 @@ static bool areStridedAccessesIndependent(uint64_t Distance, uint64_t Stride,
return ScaledDist % Stride;
}
-MemoryDepChecker::Dependence::DepType
-MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
- const MemAccessInfo &B, unsigned BIdx,
- const DenseMap<Value *, const SCEV *> &Strides) {
+/// Returns true if any of the underlying objects has a loop varying address,
+/// i.e. may change in \p L.
+static bool
+isLoopVariantIndirectAddress(ArrayRef<const Value *> UnderlyingObjects,
+ ScalarEvolution &SE, const Loop *L) {
+ return any_of(UnderlyingObjects, [&SE, L](const Value *UO) {
+ return !SE.isLoopInvariant(SE.getSCEV(const_cast<Value *>(UO)), L);
+ });
+}
+
+MemoryDepChecker::Dependence::DepType MemoryDepChecker::isDependent(
+ const MemAccessInfo &A, unsigned AIdx, const MemAccessInfo &B,
+ unsigned BIdx, const DenseMap<Value *, const SCEV *> &Strides,
+ const DenseMap<Value *, SmallVector<const Value *, 16>>
+ &UnderlyingObjects) {
assert (AIdx < BIdx && "Must pass arguments in program order");
auto [APtr, AIsWrite] = A;
@@ -1902,6 +1924,14 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
LLVM_DEBUG(dbgs() << "LAA: Distance for " << *InstMap[AIdx] << " to "
<< *InstMap[BIdx] << ": " << *Dist << "\n");
+ // Needs accesses where the addresses of the accessed underlying objects do
+ // not change within the loop.
+ if (isLoopVariantIndirectAddress(UnderlyingObjects.find(APtr)->second, SE,
+ InnermostLoop) ||
+ isLoopVariantIndirectAddress(UnderlyingObjects.find(BPtr)->second, SE,
+ InnermostLoop))
+ return Dependence::IndirectUnsafe;
+
// Need accesses with constant stride. We don't want to vectorize
// "A[B[i]] += ..." and similar code or pointer arithmetic that could wrap in
// the address space.
@@ -2064,9 +2094,11 @@ MemoryDepChecker::isDependent(const MemAccessInfo &A, unsigned AIdx,
return Dependence::BackwardVectorizable;
}
-bool MemoryDepChecker::areDepsSafe(DepCandidates &AccessSets,
- MemAccessInfoList &CheckDeps,
- const DenseMap<Value *, const SCEV *> &Strides) {
+bool MemoryDepChecker::areDepsSafe(
+ DepCandidates &AccessSets, MemAccessInfoList &CheckDeps,
+ const DenseMap<Value *, const SCEV *> &Strides,
+ const DenseMap<Value *, SmallVector<const Value *, 16>>
+ &UnderlyingObjects) {
MinDepDistBytes = -1;
SmallPtrSet<MemAccessInfo, 8> Visited;
@@ -2110,7 +2142,8 @@ bool MemoryDepChecker::areDepsSafe(DepCandidates &AccessSets,
std::swap(A, B);
Dependence::DepType Type =
- isDependent(*A.first, A.second, *B.first, B.second, Strides);
+ isDependent(*A.first, A.second, *B.first, B.second, Strides,
+ UnderlyingObjects);
mergeInStatus(Dependence::isSafeForVectorization(Type));
// Gather dependences unless we accumulated MaxDependences
@@ -2154,8 +2187,14 @@ MemoryDepChecker::getInstructionsForAccess(Value *Ptr, bool isWrite) const {
}
const char *MemoryDepChecker::Dependence::DepName[] = {
- "NoDep", "Unknown", "Forward", "ForwardButPreventsForwarding", "Backward",
- "BackwardVectorizable", "BackwardVectorizableButPreventsForwarding"};
+ "NoDep",
+ "Unknown",
+ "IndidrectUnsafe",
+ "Forward",
+ "ForwardButPreventsForwarding",
+ "Backward",
+ "BackwardVectorizable",
+ "BackwardVectorizableButPreventsForwarding"};
void MemoryDepChecker::Dependence::print(
raw_ostream &OS, unsigned Depth,
@@ -2456,7 +2495,8 @@ void LoopAccessInfo::analyzeLoop(AAResults *AA, LoopInfo *LI,
if (Accesses.isDependencyCheckNeeded()) {
LLVM_DEBUG(dbgs() << "LAA: Checking memory dependencies\n");
CanVecMem = DepChecker->areDepsSafe(
- DependentAccesses, Accesses.getDependenciesToCheck(), SymbolicStrides);
+ DependentAccesses, Accesses.getDependenciesToCheck(), SymbolicStrides,
+ Accesses.getUnderlyingObjects());
if (!CanVecMem && DepChecker->shouldRetryWithRuntimeCheck()) {
LLVM_DEBUG(dbgs() << "LAA: Retrying with memory checks\n");
@@ -2554,6 +2594,9 @@ void LoopAccessInfo::emitUnsafeDependenceRemark() {
R << "\nBackward loop carried data dependence that prevents "
"store-to-load forwarding.";
break;
+ case MemoryDepChecker::Dependence::IndirectUnsafe:
+ R << "\nUnsafe indirect dependence.";
+ break;
case MemoryDepChecker::Dependence::Unknown:
R << "\nUnknown data dependence.";
break;
diff --git a/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp b/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
index 8d75a45629486a6..5ec387300aac7f2 100644
--- a/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
+++ b/llvm/lib/Transforms/Scalar/LoopLoadElimination.cpp
@@ -195,7 +195,8 @@ class LoadEliminationForLoop {
Instruction *Source = Dep.getSource(LAI);
Instruction *Destination = Dep.getDestination(LAI);
- if (Dep.Type == MemoryDepChecker::Dependence::Unknown) {
+ if (Dep.Type == MemoryDepChecker::Dependence::Unknown ||
+ Dep.Type == MemoryDepChecker::Dependence::IndirectUnsafe) {
if (isa<LoadInst>(Source))
LoadsWithUnknownDepedence.insert(Source);
if (isa<LoadInst>(Destination))
diff --git a/llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll b/llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll
index 16759be336f896b..e643efc4bfc5c21 100644
--- a/llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll
+++ b/llvm/test/Analysis/LoopAccessAnalysis/loops-with-indirect-reads-and-writes.ll
@@ -8,8 +8,6 @@ target datalayout = "e-m:o-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:
; the loops are safe with runtime checks via FoundNonConstantDistanceDependence
; handling code in LAA.
-; FIXME: Not safe with runtime checks due to the indirect pointers are modified
-; in the loop.
define void @test_indirect_read_write_loop_also_modifies_pointer_array(ptr noundef %arr) {
; CHECK-LABEL: 'test_indirect_read_write_loop_also_modifies_pointer_array'
; CHECK-NEXT: loop.1:
@@ -23,21 +21,19 @@ define void @test_indirect_read_write_loop_also_modifies_pointer_array(ptr nound
; CHECK-EMPTY:
; CHECK-NEXT: Expressions re-written:
; CHECK-NEXT: loop.2:
-; CHECK-NEXT: Memory dependences are safe with run-time checks
+; CHECK-NEXT: Report: unsafe dependent memory operations in loop. Use #pragma clang loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
+; CHECK-NEXT: Unsafe indirect dependence.
; CHECK-NEXT: Dependences:
+; CHECK-NEXT: IndidrectUnsafe:
+; CHECK-NEXT: %l.2 = load i64, ptr %l.1, align 8, !tbaa !4 ->
+; CHECK-NEXT: store i64 %inc, ptr %l.1, align 8, !tbaa !4
+; CHECK-EMPTY:
+; CHECK-NEXT: Unknown:
+; CHECK-NEXT: %l.1 = load ptr, ptr %gep.iv.1, align 8, !tbaa !0 ->
+; CHECK-NEXT: store ptr %l.1, ptr %gep.iv.2, align 8, !tbaa !0
+; CHECK-EMPTY:
; CHECK-NEXT: Run-time memory checks:
-; CHECK-NEXT: Check 0:
-; CHECK-NEXT: Comparing group ([[GRP1:0x[0-9a-f]+]]):
-; CHECK-NEXT: %gep.iv.2 = getelementptr inbounds ptr, ptr %arr, i64 %iv.2
-; CHECK-NEXT: Against group ([[GRP2:0x[0-9a-f]+]]):
-; CHECK-NEXT: %gep.iv.1 = getelementptr inbounds ptr, ptr %arr, i64 %iv.1
; CHECK-NEXT: Grouped accesses:
-; CHECK-NEXT: Group [[GRP1]]:
-; CHECK-NEXT: (Low: {(64 + %arr),+,64}<%loop.1> High: {(8064 + %arr),+,64}<%loop.1>)
-; CHECK-NEXT: Member: {{\{\{}}(64 + %arr),+,64}<%loop.1>,+,8}<%loop.2>
-; CHECK-NEXT: Group [[GRP2]]:
-; CHECK-NEXT: (Low: %arr High: (8000 + %arr))
-; CHECK-NEXT: Member: {%arr,+,8}<nuw><%loop.2>
; CHECK-EMPTY:
; CHECK-NEXT: Non vectorizable stores to invariant address were not found in loop.
; CHECK-NEXT: SCEV assumptions:
@@ -96,15 +92,15 @@ define void @test_indirect_read_loop_also_modifies_pointer_array(ptr noundef %ar
; CHECK-NEXT: Dependences:
; CHECK-NEXT: Run-time memory checks:
; CHECK-NEXT: Check 0:
-; CHECK-NEXT: Comparing group ([[GRP3:0x[0-9a-f]+]]):
+; CHECK-NEXT: Comparing group ([[GRP1:0x[0-9a-f]+]]):
; CHECK-NEXT: %gep.iv.2 = getelementptr inbounds i64, ptr %arr, i64 %iv.2
-; CHECK-NEXT: Against group ([[GRP4:0x[0-9a-f]+]]):
+; CHECK-NEXT: Against group ([[GRP2:0x[0-9a-f]+]]):
; CHECK-NEXT: %gep.iv.1 = getelementptr inbounds ptr, ptr %arr, i64 %iv.1
; CHECK-NEXT: Grouped accesses:
-; CHECK-NEXT: Group [[GRP3]]:
+; CHECK-NEXT: Group [[GRP1]]:
; CHECK-NEXT: (Low: {(64 + %arr),+,64}<%loop.1> High: {(8064 + %arr),+,64}<%loop.1>)
; CHECK-NEXT: Member: {{\{\{}}(64 + %arr),+,64}<%loop.1>,+,8}<%loop.2>
-; CHECK-NEXT: Group [[GRP4]]:
+; CHECK-NEXT: Group [[GRP2]]:
; CHECK-NEXT: (Low: %arr High: (8000 + %arr))
; CHECK-NEXT: Member: {%arr,+,8}<nuw><%loop.2>
; CHECK-EMPTY:
@@ -164,15 +160,15 @@ define void @test_indirect_write_loop_also_modifies_pointer_array(ptr noundef %a
; CHECK-NEXT: Dependences:
; CHECK-NEXT: Run-time memory checks:
; CHECK-NEXT: Check 0:
-; CHECK-NEXT: Comparing group ([[GRP5:0x[0-9a-f]+]]):
+; CHECK-NEXT: Comparing group ([[GRP3:0x[0-9a-f]+]]):
; CHECK-NEXT: %gep.iv.2 = getelementptr inbounds ptr, ptr %arr, i64 %iv.2
-; CHECK-NEXT: Against group ([[GRP6:0x[0-9a-f]+]]):
+; CHECK-NEXT: Against group ([[GRP4:0x[0-9a-f]+]]):
; CHECK-NEXT: %gep.iv.1 = getelementptr inbounds ptr, ptr %arr, i64 %iv.1
; CHECK-NEXT: Grouped accesses:
-; CHECK-NEXT: Group [[GRP5]]:
+; CHECK-NEXT: Group [[GRP3]]:
; CHECK-NEXT: (Low: {(64 + %arr),+,64}<%loop.1> High: {(8064 + %arr),+,64}<%loop.1>)
; CHECK-NEXT: Member: {{\{\{}}(64 + %arr),+,64}<%loop.1>,+,8}<%loop.2>
-; CHECK-NEXT: Group [[GRP6]]:
+; CHECK-NEXT: Group [[GRP4]]:
; CHECK-NEXT: (Low: %arr High: (8000 + %arr))
; CHECK-NEXT: Member: {%arr,+,8}<nuw><%loop.2>
; CHECK-EMPTY:
@@ -230,21 +226,19 @@ define void @test_indirect_read_write_loop_does_not_modify_pointer_array(ptr nou
; CHECK-EMPTY:
; CHECK-NEXT: Expressions re-written:
; CHECK-NEXT: loop.2:
-; CHECK-NEXT: Memory dependences are safe with run-time checks
+; CHECK-NEXT: Report: unsafe dependent memory operations in loop. Use #pragma clang loop distribute(enable) to allow loop distribution to attempt to isolate the offending operations into a separate loop
+; CHECK-NEXT: Unsafe indirect dependence.
; CHECK-NEXT: Dependences:
+; CHECK-NEXT: IndidrectUnsafe:
+; CHECK-NEXT: %l.2 = load i64, ptr %l.1, align 8, !tbaa !4 ->
+; CHECK-NEXT: store i64 %inc, ptr %l.1, align 8, !tbaa !4
+; CHECK-EMPTY:
+; CHECK-NEXT: Unknown:
+; CHECK-NEXT: %l.3 = load i64, ptr %gep.arr2.iv.1, align 8 ->
+; CHECK-NEXT: store i64 %inc.2, ptr %gep.arr2.iv.2, align 8, !tbaa !0
+; CHECK-EMPTY:
; CHECK-NEXT: Run-time memory checks:
-; CHECK-NEXT: Check 0:
-; CHECK-NEXT: Comparing group ([[GRP7:0x[0-9a-f]+]]):
-; CHECK-NEXT: %gep.arr2.iv.2 = getelementptr inbounds i64, ptr %arr2, i64 %iv.2
-; CHECK-NEXT: Against group ([[GRP8:0x[0-9a-f]+]]):
-; CHECK-NEXT: %gep.arr2.iv.1 = getelementptr inbounds i64, ptr %arr2, i64 %iv.1
; CHECK-NEXT: Grouped accesses:
-; CHECK-NEXT: Group [[GRP7]]:
-; CHECK-NEXT: (Low: {(64 + %arr2),+,64}<%loop.1> High: {(8064 + %arr2),+,64}<%loop.1>)
-; CHECK-NEXT: Member: {{\{\{}}(64 + %arr2),+,64}<%loop.1>,+,8}<%loop.2>
-; CHECK-NEXT: Group [[GRP8]]:
-; CHECK-NEXT: (Low: %arr2 High: (8000 + %arr2))
-; CHECK-NEXT: Member: {%arr2,+,8}<nuw><%loop.2>
; CHECK-EMPTY:
; CHECK-NEXT: Non vectorizable stores to invariant address were not found in loop.
; CHECK-NEXT: SCEV assumptions:
More information about the llvm-commits
mailing list