[llvm-dev] LegacyPM: preserving passes and addRequiredTransitive

[Jay Foad via llvm-dev]

Incidentally I have just spotted this patch, which looks like it is
working around the same problem:
https://reviews.llvm.org/D87137 "[EarlyCSE] Explicitly require
AAResultsWrapperPass."

And this bug, which might be the same too:
https://bugs.llvm.org/show_bug.cgi?id=30370 "Assert in
LegacyPassManager when adding PostDominatorTree to analysis pass"

Jay.

On Fri, 27 Nov 2020 at 15:06, Jay Foad <jay.foad at gmail.com> wrote:
>
> I've been experimenting with adding passes to the codegen pipeline and
> run into a problem: if a pass P preserves analysis A, and analysis A
> "transitively requires" analysis B, then I think P should also
> preserve B.
>
> My understanding of "A transitively requires B" is that B has to be
> available, not just when A is run, but for as long as A is available.
> So I don't think it makes much sense to preserve A unless you're also
> going to preserve B, and I suggest that we add assertions to the pass
> manager to catch passes that violate this rule. (Alternatively, if a
> pass preserves A but not B, we could quietly treat it as if it didn't
> preserve A after all.)
>
> There are certainly some in-tree passes that violate the rule, but it
> seems like we mostly get away with it. I only discovered it when I
> started adding lots of extra passes into the codegen pipeline.
>
> Example: if I apply this patch to tweak the AMDGPU codegen pipeline:
>
> diff --git a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
> b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
> index ccfd62ea3e09..69928fcc4592 100644
> --- a/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
> +++ b/llvm/lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
> @@ -1019,7 +1019,9 @@ void GCNPassConfig::addOptimizedRegAlloc() {
>    // This must be run immediately after phi elimination and before
>    // TwoAddressInstructions, otherwise the processing of the tied operand of
>    // SI_ELSE will introduce a copy of the tied operand source after the else.
> +  insertPass(&PHIEliminationID, &SIFormMemoryClausesID, false);
>    insertPass(&PHIEliminationID, &SILowerControlFlowID, false);
> +  insertPass(&PHIEliminationID, &SIFormMemoryClausesID, false);
>
>    if (EnableDCEInRA)
>      insertPass(&DetectDeadLanesID, &DeadMachineInstructionElimID);
>
> Then I get:
>
> $ ~/llvm-debug/bin/llc -march=amdgcn -o /dev/null /dev/null
> llc: /home/jayfoad2/git/llvm-project/llvm/lib/IR/LegacyPassManager.cpp:657:
> void llvm::PMTopLevelManager::setLastUser(ArrayRef<llvm::Pass *>,
> llvm::Pass *): Assertion `AnalysisPass && "Expected analysis pass to
> exist."' failed.
>
> Note that it hasn't run any passes yet. This assertion fails when
> adding passes to the pass manager.
>
> The problem in this case is that SIFormMemoryClauses requires
> LiveIntervals, SILowerControlFlow preserves LiveIntervals but *not*
> MachineDominators, and LiveIntervals transitively requires
> MachineDominators.
>
> Thoughts? Are people happy with the idea of getting the pass manager
> to spot offiending passes, and complain loudly as soon as you try to
> add them to a pass manager? Obviously there would have to be a period
> of grace, to allow people to fix the offending passes.
>
> Thanks,
> Jay.