[AArch64] Don't run loop-idiom-vectorize pass in the O0 pipeline #156802
Conversation
Created using spr 1.3.5-bogner
|
@llvm/pr-subscribers-backend-aarch64 Author: Fangrui Song (MaskRay) ChangesAs noted in #156787 Full diff: https://github.com/llvm/llvm-project/pull/156802.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
index e67bd5869ccd1..4650b2d0c8151 100644
--- a/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
+++ b/llvm/lib/Target/AArch64/AArch64TargetMachine.cpp
@@ -589,7 +589,8 @@ void AArch64TargetMachine::registerPassBuilderCallbacks(PassBuilder &PB) {
PB.registerLateLoopOptimizationsEPCallback(
[=](LoopPassManager &LPM, OptimizationLevel Level) {
- LPM.addPass(LoopIdiomVectorizePass());
+ if (Level != OptimizationLevel::O0)
+ LPM.addPass(LoopIdiomVectorizePass());
});
if (getTargetTriple().isOSWindows())
PB.registerPipelineEarlySimplificationEPCallback(
|
|
I may be missing the broader context, while I do see the tagged issue. Is this purely because x86 does not run the pass? How do we decide what goes in O0? |
As little was possible. The passes that are run at -O0 are maintaining historical semantic accidents, like always-inliner and should include no optimizations I'd expect the pass builder to not call this hook in the first place, rather than requiring all targets to check this |
|
Can we add a test that checks |
| [=](LoopPassManager &LPM, OptimizationLevel Level) { | ||
| LPM.addPass(LoopIdiomVectorizePass()); | ||
| if (Level != OptimizationLevel::O0) | ||
| LPM.addPass(LoopIdiomVectorizePass()); |
There was a problem hiding this comment.
Looks like these are invoked from buildO0DefaultPipeline which is strange, but then we also call invokeCGSCCOptimizerLateEPCallbacks, invokeLoopOptimizerEndEPCallbacks and invokeScalarOptimizerLateEPCallbacks from there too.
Perhaps better to guard the entire register function with the check, i.e.
if (Level != OptimizationLevel::O0) {
PB.registerLateLoopOptimizationsEPCallback(
[=](LoopPassManager &LPM, OptimizationLevel Level) {
LPM.addPass(LoopIdiomVectorizePass());
});
}
There was a problem hiding this comment.
The Level variable is an argument to the lambda it doesn't exist outside.
There was a problem hiding this comment.
OK, that's true, but I just thought it must be easy enough to find out what the optimisation level is and just not bothering calling registerLateLoopOptimizationsEPCallback at all. I guess it's fine at the moment because we're only registering one pass.
MaskRay
left a comment
MaskRay
left a comment
There was a problem hiding this comment.
Added llvm/test/CodeGen/AArch64/print-pipeline-passes.ll . CodeGen/AArch64 doesn't test this option before.
| Core | ||
| GlobalISel | ||
| MC | ||
| Passes |
There was a problem hiding this comment.
Why is this adding a new dependency?
There was a problem hiding this comment.
OptimizationLevel::O0 is a variable defined by Passes .cpp file. This is needed to avoid an undefined symbol linker error. I don't find a way to move the definition from .cpp to .h as a static constexpr variable.
The class OptimizationLevel is not considered a literal class before }. Declararing a member constexpr variable of the same type is not allowed.
struct A {
int a, b;
constexpr A(int a, int b) : a(a), b(b) {}
static constexpr A default_value{0, 0};
};
// would cause
error: constexpr variable cannot have non-literal type 'const A'
6 | static constexpr inline A default_value{0, 0};
There was a problem hiding this comment.
I agree it does seem a shame to add the extra dependency. Does #157057 help?
There was a problem hiding this comment.
BTW, it looks like #157057 may not go anywhere, so don't let me hold up this PR.
davemgreen
left a comment
davemgreen
left a comment
There was a problem hiding this comment.
Thanks for adding the test. LGTM.
| ; RUN: opt -mtriple=aarch64 -S -passes='default<O2>' -print-pipeline-passes < %s | FileCheck %s | ||
|
|
||
| ; CHECK: loop-idiom-vectorize | ||
| ; O0-NOT: loop-idiom-vectorize |
There was a problem hiding this comment.
I was thinking of testing the entire pipeline for -O0 as it should be very small, and would prevent us from adding extra passes to it again. The other optimization levels are a bit long for that.
Sometimes code depends upon the constant variables declared in OptimizationLevel (e.g. see llvm#156802), which then requires them to add a dependency on the Passes directory. Given that code is much more likely to depend upon Support than Passes I believe it makes sense to move OptimizationLevel to Support. The Passes component already depends upon Support.
Created using spr 1.3.5-bogner
MaskRay
deleted the
users/MaskRay/spr/aarch64-dont-run-loop-idiom-vectorize-pass-in-the-o0-pipeline
branch
|
LLVM Buildbot has detected a new failure on builder Full details are available at: https://lab.llvm.org/buildbot/#/builders/141/builds/11337 Here is the relevant piece of the build log for the reference |
As noted in #156787