Skip to content

Commit d717689

Browse files
committed
[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 llvm#100721. Partially fixes llvm#130688 and a miscompile we're seeing.
1 parent 34fa037 commit d717689

File tree

2 files changed

+32
-13
lines changed

2 files changed

+32
-13
lines changed

llvm/lib/Analysis/ScalarEvolution.cpp

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -653,8 +653,7 @@ static int CompareValueComplexity(const LoopInfo *const LI, Value *LV,
653653
// If the max analysis depth was reached, return std::nullopt, assuming we do
654654
// not know if they are equivalent for sure.
655655
static std::optional<int>
656-
CompareSCEVComplexity(EquivalenceClasses<const SCEV *> &EqCacheSCEV,
657-
const LoopInfo *const LI, const SCEV *LHS,
656+
CompareSCEVComplexity(const LoopInfo *const LI, const SCEV *LHS,
658657
const SCEV *RHS, DominatorTree &DT, unsigned Depth = 0) {
659658
// Fast-path: SCEVs are uniqued so we can do a quick equality check.
660659
if (LHS == RHS)
@@ -665,9 +664,6 @@ CompareSCEVComplexity(EquivalenceClasses<const SCEV *> &EqCacheSCEV,
665664
if (LType != RType)
666665
return (int)LType - (int)RType;
667666

668-
if (EqCacheSCEV.isEquivalent(LHS, RHS))
669-
return 0;
670-
671667
if (Depth > MaxSCEVCompareDepth)
672668
return std::nullopt;
673669

@@ -681,8 +677,6 @@ CompareSCEVComplexity(EquivalenceClasses<const SCEV *> &EqCacheSCEV,
681677

682678
int X =
683679
CompareValueComplexity(LI, LU->getValue(), RU->getValue(), Depth + 1);
684-
if (X == 0)
685-
EqCacheSCEV.unionSets(LHS, RHS);
686680
return X;
687681
}
688682

@@ -747,12 +741,10 @@ CompareSCEVComplexity(EquivalenceClasses<const SCEV *> &EqCacheSCEV,
747741
return (int)LNumOps - (int)RNumOps;
748742

749743
for (unsigned i = 0; i != LNumOps; ++i) {
750-
auto X = CompareSCEVComplexity(EqCacheSCEV, LI, LOps[i], ROps[i], DT,
751-
Depth + 1);
744+
auto X = CompareSCEVComplexity(LI, LOps[i], ROps[i], DT, Depth + 1);
752745
if (X != 0)
753746
return X;
754747
}
755-
EqCacheSCEV.unionSets(LHS, RHS);
756748
return 0;
757749
}
758750

@@ -775,11 +767,9 @@ static void GroupByComplexity(SmallVectorImpl<const SCEV *> &Ops,
775767
LoopInfo *LI, DominatorTree &DT) {
776768
if (Ops.size() < 2) return; // Noop
777769

778-
EquivalenceClasses<const SCEV *> EqCacheSCEV;
779-
780770
// Whether LHS has provably less complexity than RHS.
781771
auto IsLessComplex = [&](const SCEV *LHS, const SCEV *RHS) {
782-
auto Complexity = CompareSCEVComplexity(EqCacheSCEV, LI, LHS, RHS, DT);
772+
auto Complexity = CompareSCEVComplexity(LI, LHS, RHS, DT);
783773
return Complexity && *Complexity < 0;
784774
};
785775
if (Ops.size() == 2) {

llvm/unittests/Analysis/ScalarEvolutionTest.cpp

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1706,4 +1706,33 @@ TEST_F(ScalarEvolutionsTest, ComplexityComparatorIsStrictWeakOrdering) {
17061706
});
17071707
}
17081708

1709+
TEST_F(ScalarEvolutionsTest, ComplexityComparatorIsStrictWeakOrdering2) {
1710+
// Regression test for a case where caching of equivalent values caused the
1711+
// comparator to get inconsistent.
1712+
1713+
Type *Int64Ty = Type::getInt64Ty(Context);
1714+
Type *PtrTy = PointerType::get(Context, 0);
1715+
FunctionType *FTy = FunctionType::get(Type::getVoidTy(Context),
1716+
{PtrTy, PtrTy, PtrTy, Int64Ty}, false);
1717+
Function *F = Function::Create(FTy, Function::ExternalLinkage, "f", M);
1718+
BasicBlock *BB = BasicBlock::Create(Context, "entry", F);
1719+
ReturnInst::Create(Context, nullptr, BB);
1720+
1721+
ScalarEvolution SE = buildSE(*F);
1722+
1723+
const SCEV *S0 = SE.getSCEV(F->getArg(0));
1724+
const SCEV *S1 = SE.getSCEV(F->getArg(1));
1725+
const SCEV *S2 = SE.getSCEV(F->getArg(2));
1726+
1727+
const SCEV *P0 = SE.getPtrToIntExpr(S0, Int64Ty);
1728+
const SCEV *P1 = SE.getPtrToIntExpr(S1, Int64Ty);
1729+
const SCEV *P2 = SE.getPtrToIntExpr(S2, Int64Ty);
1730+
1731+
const SCEV *M0 = SE.getNegativeSCEV(P0);
1732+
const SCEV *M2 = SE.getNegativeSCEV(P2);
1733+
1734+
SmallVector<const SCEV *, 6> Ops = {M2, P0, M0, P1, P2};
1735+
SE.getAddExpr(Ops);
1736+
}
1737+
17091738
} // end namespace llvm

0 commit comments

Comments
 (0)