From 66825de5bb2c4681c3a3dbbefc487391f8ac0cae Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Thu, 29 Aug 2024 12:21:20 +0100 Subject: [PATCH 1/2] [LAA] Be more careful when evaluating AddRecs at symbolic max BTC. Evaluating AR at the symbolic max BTC may wrap and create an expression that is less than the start of the AddRec due to wrapping (for example consider MaxBTC = -2). If that's the case, set ScEnd to -(EltSize + 1). ScEnd will get incremented by EltSize before returning, so this effectively sets ScEnd to unsigned max. Note that LAA separately checks that accesses cannot not wrap, so unsigned max represents an upper bound. --- llvm/lib/Analysis/LoopAccessAnalysis.cpp | 25 ++++++++++++++----- ...bolic-max-backedge-taken-count-may-wrap.ll | 6 ++--- 2 files changed, 21 insertions(+), 10 deletions(-) diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp index 980f142f11326..a06490fdd06fa 100644 --- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp +++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp @@ -219,13 +219,29 @@ static std::pair getStartAndEndForAccess( const SCEV *ScStart; const SCEV *ScEnd; + auto &DL = Lp->getHeader()->getDataLayout(); + Type *IdxTy = DL.getIndexType(PtrExpr->getType()); + const SCEV *EltSizeSCEV = SE->getStoreSizeOfExpr(IdxTy, AccessTy); if (SE->isLoopInvariant(PtrExpr, Lp)) { ScStart = ScEnd = PtrExpr; } else if (auto *AR = dyn_cast(PtrExpr)) { - const SCEV *Ex = PSE.getSymbolicMaxBackedgeTakenCount(); - ScStart = AR->getStart(); - ScEnd = AR->evaluateAtIteration(Ex, *SE); + const SCEV *BTC = PSE.getBackedgeTakenCount(); + if (!isa(BTC)) + ScEnd = AR->evaluateAtIteration(BTC, *SE); + else { + // Evaluating AR at MaxBTC may wrap and create an expression that is less + // than the start of the AddRec due to wrapping (for example consider + // MaxBTC = -2). If that's the case, set ScEnd to -(EltSize + 1). ScEnd + // will get incremented by EltSize before returning, so this effectively + // sets ScEnd to unsigned max. Note that LAA separately checks that + // accesses cannot not wrap, so unsigned max represents an upper bound. + const SCEV *MaxBTC = PSE.getSymbolicMaxBackedgeTakenCount(); + ScEnd = AR->evaluateAtIteration(MaxBTC, *SE); + if (!SE->isKnownNonNegative(SE->getMinusSCEV(ScEnd, ScStart))) + ScEnd = SE->getNegativeSCEV( + SE->getAddExpr(EltSizeSCEV, SE->getOne(EltSizeSCEV->getType()))); + } const SCEV *Step = AR->getStepRecurrence(*SE); // For expressions with negative step, the upper bound is ScStart and the @@ -247,9 +263,6 @@ static std::pair getStartAndEndForAccess( assert(SE->isLoopInvariant(ScEnd, Lp)&& "ScEnd needs to be invariant"); // Add the size of the pointed element to ScEnd. - auto &DL = Lp->getHeader()->getDataLayout(); - Type *IdxTy = DL.getIndexType(PtrExpr->getType()); - const SCEV *EltSizeSCEV = SE->getStoreSizeOfExpr(IdxTy, AccessTy); ScEnd = SE->getAddExpr(ScEnd, EltSizeSCEV); Iter->second = {ScStart, ScEnd}; diff --git a/llvm/test/Analysis/LoopAccessAnalysis/evaluate-at-symbolic-max-backedge-taken-count-may-wrap.ll b/llvm/test/Analysis/LoopAccessAnalysis/evaluate-at-symbolic-max-backedge-taken-count-may-wrap.ll index b9951c7ed02ec..6da807749ab90 100644 --- a/llvm/test/Analysis/LoopAccessAnalysis/evaluate-at-symbolic-max-backedge-taken-count-may-wrap.ll +++ b/llvm/test/Analysis/LoopAccessAnalysis/evaluate-at-symbolic-max-backedge-taken-count-may-wrap.ll @@ -3,7 +3,6 @@ target datalayout = "e-m:e-p:32:32-Fi8-i64:64-v128:64:128-a:0:32-n32-S64" -; FIXME: Start == End for access group with AddRec. define void @runtime_checks_with_symbolic_max_btc_neg_1(ptr %P, ptr %S, i32 %x, i32 %y) { ; CHECK-LABEL: 'runtime_checks_with_symbolic_max_btc_neg_1' ; CHECK-NEXT: loop: @@ -17,7 +16,7 @@ define void @runtime_checks_with_symbolic_max_btc_neg_1(ptr %P, ptr %S, i32 %x, ; CHECK-NEXT: ptr %S ; CHECK-NEXT: Grouped accesses: ; CHECK-NEXT: Group [[GRP1]]: -; CHECK-NEXT: (Low: ((4 * %y) + %P) High: ((4 * %y) + %P)) +; CHECK-NEXT: (Low: ((4 * %y) + %P) High: -1) ; CHECK-NEXT: Member: {((4 * %y) + %P),+,4}<%loop> ; CHECK-NEXT: Group [[GRP2]]: ; CHECK-NEXT: (Low: %S High: (4 + %S)) @@ -44,7 +43,6 @@ exit: ret void } -; FIXME: Start > End for access group with AddRec. define void @runtime_check_with_symbolic_max_btc_neg_2(ptr %P, ptr %S, i32 %x, i32 %y) { ; CHECK-LABEL: 'runtime_check_with_symbolic_max_btc_neg_2' ; CHECK-NEXT: loop: @@ -58,7 +56,7 @@ define void @runtime_check_with_symbolic_max_btc_neg_2(ptr %P, ptr %S, i32 %x, i ; CHECK-NEXT: ptr %S ; CHECK-NEXT: Grouped accesses: ; CHECK-NEXT: Group [[GRP3]]: -; CHECK-NEXT: (Low: ((4 * %y) + %P) High: (-4 + (4 * %y) + %P)) +; CHECK-NEXT: (Low: ((4 * %y) + %P) High: -1) ; CHECK-NEXT: Member: {((4 * %y) + %P),+,4}<%loop> ; CHECK-NEXT: Group [[GRP4]]: ; CHECK-NEXT: (Low: %S High: (4 + %S)) From c03735059e21355fa7b31425a3986bd9e04080d4 Mon Sep 17 00:00:00 2001 From: Florian Hahn Date: Thu, 29 Aug 2024 20:24:58 +0100 Subject: [PATCH 2/2] !fixup docment why evaluating at BTC should be safe --- llvm/lib/Analysis/LoopAccessAnalysis.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/llvm/lib/Analysis/LoopAccessAnalysis.cpp b/llvm/lib/Analysis/LoopAccessAnalysis.cpp index a06490fdd06fa..2fdfc9be14f33 100644 --- a/llvm/lib/Analysis/LoopAccessAnalysis.cpp +++ b/llvm/lib/Analysis/LoopAccessAnalysis.cpp @@ -228,6 +228,10 @@ static std::pair getStartAndEndForAccess( ScStart = AR->getStart(); const SCEV *BTC = PSE.getBackedgeTakenCount(); if (!isa(BTC)) + // Evaluating AR at an exact BTC is safe: LAA separately checks that + // accesses cannot wrap in the loop. If evaluating AR at BTC wraps, then + // the loop either triggers UB when executing a memory access with a + // poison pointer or the wrapping/poisoned pointer is not used. ScEnd = AR->evaluateAtIteration(BTC, *SE); else { // Evaluating AR at MaxBTC may wrap and create an expression that is less