[SimpleLoopUnswitch][NFC] move quadratic asserts under EXPENSIVE_CHECKS #144887
Conversation
|
@llvm/pr-subscribers-llvm-transforms Author: None (jeanPerier) ChangesThree asserts/checks in SimpleLoopUnswitchPass are quite expensive on IR containing many deeply nested loop nests as in repro_60_loops_depth_10.ll.txt. Their cost is not linear with the number of loop nest and these asserts quickly become the most significant cost of the whole compilation. This patch moves these asserts under This problem is easily exposed with flang because of Fortran multidimensional arrays and array expressions: For instance, the code below generates 5 loop nests of depth 10. Here is the time in second and the percentage of the whole LLVM optimization time spent in SimpleLoopUnswitchPass before and after this patch with Before: After: As you can see the assert cost is not linear with the number of loops and 75s out of 81s are spent in these asserts for N=100. repro_60_loops_depth_10.ll.txt is the .ll generated by flang for the N=60 case to illustrate and repoduce without flang. Full diff: https://github.com/llvm/llvm-project/pull/144887.diff 1 Files Affected:
diff --git a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
index 9b40fc03da6bb..675f6e1975535 100644
--- a/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
+++ b/llvm/lib/Transforms/Scalar/SimpleLoopUnswitch.cpp
@@ -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));
+#endif
if (BI && !PartiallyInvariant) {
// If we unswitched a branch which collapses the condition to a known
@@ -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);
@@ -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)
|
| @@ -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)); | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
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.
|
Looks good to me, though, I am not familiar with the code and cannot comment on @antoniofrighetto's concern. |
Three asserts/checks in SimpleLoopUnswitchPass are quite expensive on IR containing many deeply nested loop nests as in repro_60_loops_depth_10.ll.txt. Their cost is not linear with the number of loop nest and these asserts quickly become the most significant cost of the whole compilation.
This patch moves these asserts under
EXPENSIVE_CHECKS.This problem is easily exposed with flang because of Fortran multidimensional arrays and array expressions:
For instance, the code below generates 5 loop nests of depth 10.
Here is the time in second and the percentage of the whole LLVM optimization time spent in SimpleLoopUnswitchPass before and after this patch with
flang -O3, with N the number of variable/loop nests created:Before:
After:
As you can see the assert cost is not linear with the number of loops and 75s out of 81s are spent in these asserts for N=100.
repro_60_loops_depth_10.ll.txt is the .ll generated by flang for the N=60 case to illustrate and repoduce without flang.