From d717689ba7234f8a421d2ffada19d2943aba4f6f Mon Sep 17 00:00:00 2001 From: Arthur Eubanks Date: Thu, 27 Mar 2025 00:23:50 +0000 Subject: [PATCH 1/2] [SCEV] Remove EqCacheSCEV This was added in https://reviews.llvm.org/D26389 to help with extremely deep SCEV expressions. However, this is wrong since we may cache sub-SCEVs to be equivalent that CompareValueComplexity returned 0 due to hitting the max comparison depth. This doesn't fully fix the issue, since we may still consider deeply nested SCEVs that only differ in the innermost expression as equivalent due to hitting depth limits, but this helps with part of the problem. This also improves compile time in some compiles: https://llvm-compile-time-tracker.com/compare.php?from=34fa037c4fd7f38faada5beedc63ad234e904247&to=e241ecf999f4dd42d4b951d4a5d4f8eabeafcff0&stat=instructions:u Similar to #100721. Partially fixes #130688 and a miscompile we're seeing. --- llvm/lib/Analysis/ScalarEvolution.cpp | 16 ++-------- .../Analysis/ScalarEvolutionTest.cpp | 29 +++++++++++++++++++ 2 files changed, 32 insertions(+), 13 deletions(-) diff --git a/llvm/lib/Analysis/ScalarEvolution.cpp b/llvm/lib/Analysis/ScalarEvolution.cpp index 314baa7c7aee1..82e46e8800b3a 100644 --- a/llvm/lib/Analysis/ScalarEvolution.cpp +++ b/llvm/lib/Analysis/ScalarEvolution.cpp @@ -653,8 +653,7 @@ static int CompareValueComplexity(const LoopInfo *const LI, Value *LV, // If the max analysis depth was reached, return std::nullopt, assuming we do // not know if they are equivalent for sure. static std::optional -CompareSCEVComplexity(EquivalenceClasses &EqCacheSCEV, - const LoopInfo *const LI, const SCEV *LHS, +CompareSCEVComplexity(const LoopInfo *const LI, const SCEV *LHS, const SCEV *RHS, DominatorTree &DT, unsigned Depth = 0) { // Fast-path: SCEVs are uniqued so we can do a quick equality check. if (LHS == RHS) @@ -665,9 +664,6 @@ CompareSCEVComplexity(EquivalenceClasses &EqCacheSCEV, if (LType != RType) return (int)LType - (int)RType; - if (EqCacheSCEV.isEquivalent(LHS, RHS)) - return 0; - if (Depth > MaxSCEVCompareDepth) return std::nullopt; @@ -681,8 +677,6 @@ CompareSCEVComplexity(EquivalenceClasses &EqCacheSCEV, int X = CompareValueComplexity(LI, LU->getValue(), RU->getValue(), Depth + 1); - if (X == 0) - EqCacheSCEV.unionSets(LHS, RHS); return X; } @@ -747,12 +741,10 @@ CompareSCEVComplexity(EquivalenceClasses &EqCacheSCEV, return (int)LNumOps - (int)RNumOps; for (unsigned i = 0; i != LNumOps; ++i) { - auto X = CompareSCEVComplexity(EqCacheSCEV, LI, LOps[i], ROps[i], DT, - Depth + 1); + auto X = CompareSCEVComplexity(LI, LOps[i], ROps[i], DT, Depth + 1); if (X != 0) return X; } - EqCacheSCEV.unionSets(LHS, RHS); return 0; } @@ -775,11 +767,9 @@ static void GroupByComplexity(SmallVectorImpl &Ops, LoopInfo *LI, DominatorTree &DT) { if (Ops.size() < 2) return; // Noop - EquivalenceClasses EqCacheSCEV; - // Whether LHS has provably less complexity than RHS. auto IsLessComplex = [&](const SCEV *LHS, const SCEV *RHS) { - auto Complexity = CompareSCEVComplexity(EqCacheSCEV, LI, LHS, RHS, DT); + auto Complexity = CompareSCEVComplexity(LI, LHS, RHS, DT); return Complexity && *Complexity < 0; }; if (Ops.size() == 2) { diff --git a/llvm/unittests/Analysis/ScalarEvolutionTest.cpp b/llvm/unittests/Analysis/ScalarEvolutionTest.cpp index c72cecbba3cb8..84a958f2c95e2 100644 --- a/llvm/unittests/Analysis/ScalarEvolutionTest.cpp +++ b/llvm/unittests/Analysis/ScalarEvolutionTest.cpp @@ -1706,4 +1706,33 @@ TEST_F(ScalarEvolutionsTest, ComplexityComparatorIsStrictWeakOrdering) { }); } +TEST_F(ScalarEvolutionsTest, ComplexityComparatorIsStrictWeakOrdering2) { + // Regression test for a case where caching of equivalent values caused the + // comparator to get inconsistent. + + Type *Int64Ty = Type::getInt64Ty(Context); + Type *PtrTy = PointerType::get(Context, 0); + FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context), + {PtrTy, PtrTy, PtrTy, Int64Ty}, false); + Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", M); + BasicBlock *BB = BasicBlock::Create(Context, "entry", F); + ReturnInst::Create(Context, nullptr, BB); + + ScalarEvolution SE = buildSE(*F); + + const SCEV *S0 = SE.getSCEV(F->getArg(0)); + const SCEV *S1 = SE.getSCEV(F->getArg(1)); + const SCEV *S2 = SE.getSCEV(F->getArg(2)); + + const SCEV *P0 = SE.getPtrToIntExpr(S0, Int64Ty); + const SCEV *P1 = SE.getPtrToIntExpr(S1, Int64Ty); + const SCEV *P2 = SE.getPtrToIntExpr(S2, Int64Ty); + + const SCEV *M0 = SE.getNegativeSCEV(P0); + const SCEV *M2 = SE.getNegativeSCEV(P2); + + SmallVector Ops = {M2, P0, M0, P1, P2}; + SE.getAddExpr(Ops); +} + } // end namespace llvm From 9bbae9c6a3575b21076dbf487d691a8a1501cc9a Mon Sep 17 00:00:00 2001 From: Arthur Eubanks Date: Thu, 27 Mar 2025 00:39:44 +0000 Subject: [PATCH 2/2] add comment to test --- llvm/unittests/Analysis/ScalarEvolutionTest.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/llvm/unittests/Analysis/ScalarEvolutionTest.cpp b/llvm/unittests/Analysis/ScalarEvolutionTest.cpp index 84a958f2c95e2..95a4affdd7789 100644 --- a/llvm/unittests/Analysis/ScalarEvolutionTest.cpp +++ b/llvm/unittests/Analysis/ScalarEvolutionTest.cpp @@ -1732,6 +1732,8 @@ TEST_F(ScalarEvolutionsTest, ComplexityComparatorIsStrictWeakOrdering2) { const SCEV *M2 = SE.getNegativeSCEV(P2); SmallVector Ops = {M2, P0, M0, P1, P2}; + // When _LIBCPP_HARDENING_MODE == _LIBCPP_HARDENING_MODE_DEBUG, this will + // crash if the comparator has the specific caching bug. SE.getAddExpr(Ops); }