Skip to content

Commit 47021bd

Browse files
committed
[IndVars] Option verify-indvars is broken (and always has been), delete it
This option is switched off by default, and it seems that it has never worked correctly. What it basically does is: it remembers current BECount SCEV, and after all transforms tries to validate some facts for it. However, between these two points this SCEV may become invalid (e.g. because some SCEVUnknown it references may be deleted as dead code). So basically it may work with broken pointers. Besides, its implementation does strange things (e.g. forgetLoop) which are invasive and may affect behavior in other parts of the system (specifically verification), concealing some other problems. Another issue is that it may use SCEVCouldNotCompute object without checking this. The option is not used in any unit tests, and if switched on by default, the following tests fail: ``` ******************** Failed Tests (14): LLVM :: Transforms/IndVarSimplify/2005-06-15-InstMoveCrash.ll LLVM :: Transforms/IndVarSimplify/2007-06-06-DeleteDanglesPtr.ll LLVM :: Transforms/IndVarSimplify/2008-10-03-CouldNotCompute.ll LLVM :: Transforms/IndVarSimplify/2009-05-24-useafterfree.ll LLVM :: Transforms/IndVarSimplify/2011-10-27-lftrnull.ll LLVM :: Transforms/IndVarSimplify/ARM/code-size.ll LLVM :: Transforms/IndVarSimplify/X86/deterministic-scev-verify.ll LLVM :: Transforms/IndVarSimplify/X86/pr57187.ll LLVM :: Transforms/IndVarSimplify/X86/verify-scev.ll LLVM :: Transforms/IndVarSimplify/bbi-63564.ll LLVM :: Transforms/IndVarSimplify/invalidate-modified-lcssa-phi.ll LLVM :: Transforms/IndVarSimplify/loop-predication.ll LLVM :: Transforms/IndVarSimplify/post-inc-range.ll LLVM :: Transforms/IndVarSimplify/turn-to-invariant.ll ******************** Unexpectedly Passed Tests (1): LLVM :: Transforms/IndVarSimplify/pr55689.ll ``` None of these looks like real problems found by verification, these are bugs in the verifying code itself (such as use of deleted SCEVs and SCEVCouldNotCompute's). I think it all gives enough justification for its removal. #61302 Differential Revision: https://reviews.llvm.org/D145894 Reviewed By: nikic
1 parent e4ea2d5 commit 47021bd

File tree

1 file changed

+1
-37
lines changed

1 file changed

+1
-37
lines changed

llvm/lib/Transforms/Scalar/IndVarSimplify.cpp

Lines changed: 1 addition & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -93,15 +93,6 @@ STATISTIC(NumLFTR , "Number of loop exit tests replaced");
9393
STATISTIC(NumElimExt , "Number of IV sign/zero extends eliminated");
9494
STATISTIC(NumElimIV , "Number of congruent IVs eliminated");
9595

96-
// Trip count verification can be enabled by default under NDEBUG if we
97-
// implement a strong expression equivalence checker in SCEV. Until then, we
98-
// use the verify-indvars flag, which may assert in some cases.
99-
static cl::opt<bool> VerifyIndvars(
100-
"verify-indvars", cl::Hidden,
101-
cl::desc("Verify the ScalarEvolution result after running indvars. Has no "
102-
"effect in release builds. (Note: this adds additional SCEV "
103-
"queries potentially changing the analysis result)"));
104-
10596
static cl::opt<ReplaceExitVal> ReplaceExitValue(
10697
"replexitval", cl::Hidden, cl::init(OnlyCheapRepl),
10798
cl::desc("Choose the strategy to replace exit value in IndVarSimplify"),
@@ -2023,16 +2014,6 @@ bool IndVarSimplify::run(Loop *L) {
20232014
if (!L->isLoopSimplifyForm())
20242015
return false;
20252016

2026-
#ifndef NDEBUG
2027-
// Used below for a consistency check only
2028-
// Note: Since the result returned by ScalarEvolution may depend on the order
2029-
// in which previous results are added to its cache, the call to
2030-
// getBackedgeTakenCount() may change following SCEV queries.
2031-
const SCEV *BackedgeTakenCount;
2032-
if (VerifyIndvars)
2033-
BackedgeTakenCount = SE->getBackedgeTakenCount(L);
2034-
#endif
2035-
20362017
bool Changed = false;
20372018
// If there are any floating-point recurrences, attempt to
20382019
// transform them to use integer recurrences.
@@ -2178,27 +2159,10 @@ bool IndVarSimplify::run(Loop *L) {
21782159
// Clean up dead instructions.
21792160
Changed |= DeleteDeadPHIs(L->getHeader(), TLI, MSSAU.get());
21802161

2162+
#ifndef NDEBUG
21812163
// Check a post-condition.
21822164
assert(L->isRecursivelyLCSSAForm(*DT, *LI) &&
21832165
"Indvars did not preserve LCSSA!");
2184-
2185-
// Verify that LFTR, and any other change have not interfered with SCEV's
2186-
// ability to compute trip count. We may have *changed* the exit count, but
2187-
// only by reducing it.
2188-
#ifndef NDEBUG
2189-
if (VerifyIndvars && !isa<SCEVCouldNotCompute>(BackedgeTakenCount)) {
2190-
SE->forgetLoop(L);
2191-
const SCEV *NewBECount = SE->getBackedgeTakenCount(L);
2192-
if (SE->getTypeSizeInBits(BackedgeTakenCount->getType()) <
2193-
SE->getTypeSizeInBits(NewBECount->getType()))
2194-
NewBECount = SE->getTruncateOrNoop(NewBECount,
2195-
BackedgeTakenCount->getType());
2196-
else
2197-
BackedgeTakenCount = SE->getTruncateOrNoop(BackedgeTakenCount,
2198-
NewBECount->getType());
2199-
assert(!SE->isKnownPredicate(ICmpInst::ICMP_ULT, BackedgeTakenCount,
2200-
NewBECount) && "indvars must preserve SCEV");
2201-
}
22022166
if (VerifyMemorySSA && MSSAU)
22032167
MSSAU->getMemorySSA()->verifyMemorySSA();
22042168
#endif

0 commit comments

Comments
 (0)