Skip to content

[SimpleLoopUnswitch][NFC] move quadratic asserts under EXPENSIVE_CHECKS #144887

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[SimpleLoopUnswitch][NFC] move quadratic asserts under EXPENSIVE_CHECKS #144887

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 5 additions & 1 deletion llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2522,12 +2522,14 @@ static void unswitchNontrivialInvariants(
if (MSSAU && VerifyMemorySSA)
MSSAU->getMemorySSA()->verifyMemorySSA();

#ifdef EXPENSIVE_CHECKS
// This transformation has a high risk of corrupting the dominator tree, and
// the below steps to rebuild loop structures will result in hard to debug
// errors in that case so verify that the dominator tree is sane first.
// FIXME: Remove this when the bugs stop showing up and rely on existing
// verification steps.
assert(DT.verify(DominatorTree::VerificationLevel::Fast));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure about ifdef'ing this one, generally considering some of the past bugs (and the comment above). Maybe the final one instead could switch to VerificationLevel::Full, w/ expensive checks enabled.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just to clarify, are you suggesting completely deleting this one because it has not fired recently and to turn the one on line 3714/3717 to VerificationLevel::Full under expensive checks, or do you want to keep this one outside of EXPENSIVE_CHECKS and use VerificationLevel::Full for the last one under EXPENSIVE_CHECKS?

The comment "Remove this when the bugs stop showing up and rely on existing verification steps" from 7 years ago seems to imply the former assuming the assert has not fired recently (not familiar with the code here, do you know? Searching GitHub issues for this asserts, I did not find an issues for this pass).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was suggesting leaving this check outside of EXPENSIVE_CHECKS. While no related issues seem to have come up recently (some of the recent bugs were unrelated to breaking the dominator tree), the pass still does rewrite loops, and in general the CFG, quite aggressively, so I'd rather keep the assert than dropping it. That said, maybe enabling it only under EXPENSIVE_CHECKS is okay...

and to turn the one on line 3714/3717 to VerificationLevel::Full under expensive checks

Yeah I think now switching this one to VerificationLevel::Full under expensive checks could be reasonable.

#endif

if (BI && !PartiallyInvariant) {
// If we unswitched a branch which collapses the condition to a known
Expand Down Expand Up @@ -2633,7 +2635,7 @@ static void unswitchNontrivialInvariants(
OuterL = OuterL->getParentLoop())
UpdateLoop(*OuterL);

#ifndef NDEBUG
#ifdef EXPENSIVE_CHECKS
// Verify the entire loop structure to catch any incorrect updates before we
// progress in the pass pipeline.
LI.verify(DT);
Expand Down Expand Up @@ -3709,9 +3711,11 @@ PreservedAnalyses SimpleLoopUnswitchPass::run(Loop &L, LoopAnalysisManager &AM,
if (AR.MSSA && VerifyMemorySSA)
AR.MSSA->verifyMemorySSA();

#ifdef EXPENSIVE_CHECKS
// Historically this pass has had issues with the dominator tree so verify it
// in asserts builds.
assert(AR.DT.verify(DominatorTree::VerificationLevel::Fast));
#endif

auto PA = getLoopPassPreservedAnalyses();
if (AR.MSSA)
Expand Down
Loading