[PATCH] D115052: [Passes] Only run extra vector passes if loops have been vectorized.

[Florian Hahn via Phabricator via llvm-commits]

fhahn added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Vectorize/LoopVectorize.h:98
+/// extra simplifications could be beneficial).
+class ExtraVectorPassManager : public FunctionPassManager {
+  /// Set of passes to run conditionally.
----------------
aeubanks wrote:
> can `ExtraVectorPassManager` only contain `ConditionalPasses` and we have a separate FPM for the normal passes? separation of concerns, seems like this is doing two things that could be separated
The reason why `ExtraVectorPassManager` contains both the conditional and unconditional passes is that this way we can check whether `ShouldRunExtraVectorPasses` is available *before* the unconditional passes run.

If we would run the unconditional passes outside of `ExtraVectorPassManager`, then any change they make would invalidate `ShouldRunExtraVectorPasses` IIUC. Unless we teach all those passes to preserve it. Or perhaps there's a different alternative?


================
Comment at: llvm/test/Other/opt-pipeline-vector-passes.ll:3
+; RUN: opt -disable-verify -debug-pass-manager -passes='default<O2>' -force-vector-width=4 -S %s 2>&1 | FileCheck %s --check-prefixes=O2
+; RUN: opt -disable-verify -debug-pass-manager -passes='default<O2>' -force-vector-width=0 -extra-vectorizer-passes -S %s 2>&1 | FileCheck %s --check-prefixes=O2
+; RUN: opt -disable-verify -debug-pass-manager -passes='default<O2>' -force-vector-width=4 -extra-vectorizer-passes -S %s 2>&1 | FileCheck %s --check-prefixes=O2_EXTRA
----------------
aeubanks wrote:
> this should check that we actually don't run the extra passes when there's no vectorization, e.g.
> `O2-NOT: Running pass: ...`
Yes, the original test doesn't really check for that. I'll push a commit to improve the test in that regard separately and will also include the vector loop.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D115052/new/

https://reviews.llvm.org/D115052