[llvm-dev] LegacyPM: preserving passes and addRequiredTransitive

[Jay Foad via llvm-dev]

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.