[CodeGen] Define an interface for the new pass manager.

[czhang]

rebase & clang-format

[czhang]

ping.

[arsenm]

llvm/include/llvm/CodeGen/PassManager.h

111

I thought it was OK for function passes to add new functions, just not delete

[fedor.sergeev]

This definitely needs unit tests.
Please, check llvm/unittests/IR/PassManagerTest.cpp for Function/Module unit tests, and, say, llvm/unittest/Analysis/LoopPassManagerTest.cpp for Loop unit tests.

llvm/include/llvm/CodeGen/PassManager.h

87–89

If you do want to emit size remarks after each machine-function pass then its best to be done through pass instrumentation.
Since you do not implement pass instrumentation right now, skip this code as well.

[fedor.sergeev]

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

In particular:

• how MachineFunction is being produced
• when MachineFunction is invalidated
• how MachineFunctions are being iterated (any particular order required etc)
• what is the "container" relation of MachineFunction to other IRUnits

Currently we have an hierarchical structure of IRUnits, with Module "containing" Functions, Function "containing" Loops, CGSCC "containing" Functions and contained in the Module.
Container is a rather principal relation here, which in particular defines the way analysis proxies are organized (Inner and Outer ones).

Currently in your code Function contains (a single) MachineFunction.
And iteration through MachineFunctions happens "by Function" (through FunctionToMachinePassAdaptor).
I see nothing wrong with this solution, just as it is a very principal decision I would better see it mentioned somewhere in high-level comments.
And then it seems to be more akin to Loops, so I would recommend to take a look at what LoopPassManager and FunctionToLoopPassAdaptor are doing.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

[paquette]

llvm/include/llvm/CodeGen/PassManager.h

87–89

Size remarks aren't supported in the new PM at all right now. I'm not entirely sure that this would work. (Testcase?)

It would be nice to have a port that would work for both codegen and IR-level passes though. :)

[philip.pfaffe]

llvm/include/llvm/CodeGen/PassManager.h

87–89

I'm not sure I understand this comment. Can you be specific in what you think the issue is currently?

[czhang]

In D64179#1606205, @fedor.sergeev wrote:

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

In particular:

• how MachineFunction is being produced

MachineFunction is produced from the MachineModuleInfo analysis on Functions. For the purposes of this patch, it doesn't really matter where the MachineFunction comes from. However, the next patch in the series addresses this, since the MachineModuleInfo analysis produces MachineFunctions for Functions in the legacy pass manager (and that change keeps it that way).

• when MachineFunction is invalidated

MachineFunctions do not get invalidated. The contract is that MachineFunction passes do not mutate their containing Functions.

• how MachineFunctions are being iterated (any particular order required etc)

MachineFunctions are never the result of any iteration. Modules are iterators yielding Functions, and MachineFunctions are associated with Functions. So any kind of ordering is induced from the ordering of Functions in a Module, which is all modeled explicitly with the adaptors. Since no implicit ordering is introduced with this change, there is not much to say about it which isn't said already by the ModuleToFunctionPassAdaptor.

• what is the "container" relation of MachineFunction to other IRUnits

Currently we have an hierarchical structure of IRUnits, with Module "containing" Functions, Function "containing" Loops, CGSCC "containing" Functions and contained in the Module.
Container is a rather principal relation here, which in particular defines the way analysis proxies are organized (Inner and Outer ones).

Yes, the relationship between MachineFunction and Function is currently a little awkwardly modeled in LLVM. MachineFunctions are treated like loops in the sense that they must be queried from a Function analysis.

Currently in your code Function contains (a single) MachineFunction.
And iteration through MachineFunctions happens "by Function" (through FunctionToMachinePassAdaptor).
I see nothing wrong with this solution, just as it is a very principal decision I would better see it mentioned somewhere in high-level comments.
And then it seems to be more akin to Loops, so I would recommend to take a look at what LoopPassManager and FunctionToLoopPassAdaptor are doing.

Ditto. Except it's not just my code that treats MachineFunctions as being contained in a Function. The legacy pass manager also treated MachineFunction as being contained inside Functions. Of course, that doesn't necessarily mean we need to repeat that design decision, which may just be an artifact of how the inheritance model worked, but enough of the existing code in CodeGen outside of the pass manager assumes such a container relationship, so this is the least intrusive way to model this.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

This sentence doesn't quite parse for me. Could you please elaborate what you mean here? Which part of the basics specifically?

[fedor.sergeev]

In D64179#1610998, @czhang wrote:

In D64179#1606205, @fedor.sergeev wrote:

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

Okey, everything you said there makes perfect sense. Thanks for clarification.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

This sentence doesn't quite parse for me. Could you please elaborate what you mean here? Which part of the basics specifically?

I mean both should work :)
In particular, MachineFunctionPassManager.
It seems that you just defined a typedef in a hope that it will work... for one you need some tests to verify that.
And my guess is that as soon as you instantiate MachineFunctionPassManager::run you will hit missing interfaces that generic PassManager template refers to.
At least I'm sure you will hit PassInstrumentation ones.

Lets first start with some basic unittests (see llvm/unittests/IR/PassManagerTest.cpp etc).

[fedor.sergeev]

Which part of the basics specifically?

Also, avoid adding anything into the adaptor except just basic functionality of passing control to the target pass.
(say, as I commented already - skip remarks stuff altogether). This commit better contain the general comments on what MachineFunction is
and basics required for PassManager/Adaptor to start working, and nothing else.
Since you will have to add PassInstrumentation interfaces in order for PassManager to operate you might be able to add its usage into PassAdaptor as well.
But that could be done as a follow up change.

[czhang]

@fedor.sergeev

In D64179#1623993, @fedor.sergeev wrote:

In D64179#1610998, @czhang wrote:

In D64179#1606205, @fedor.sergeev wrote:

I believe in order to make progress here we first need to settle down on a very principal matter -

what kind of IRUnit MachineFunction is?

Okey, everything you said there makes perfect sense. Thanks for clarification.

There are many details to dive in, but first lets work on doing the basics right:

MachineFunctionPassManager
FunctionToMachineFunctionPassAdaptor

This sentence doesn't quite parse for me. Could you please elaborate what you mean here? Which part of the basics specifically?

I mean both should work :)
In particular, MachineFunctionPassManager.
It seems that you just defined a typedef in a hope that it will work... for one you need some tests to verify that.
And my guess is that as soon as you instantiate MachineFunctionPassManager::run you will hit missing interfaces that generic PassManager template refers to.
At least I'm sure you will hit PassInstrumentation ones.

Lets first start with some basic unittests (see llvm/unittests/IR/PassManagerTest.cpp etc).

But everything does work :). The reason I know is because the 6 follow up patches after this allow llc to run and produce bitwise identical inputs for the 5 other passes I ported after this compared to the legacy pass manager. (using --run-new-pass after the prerequisite passes are manually run with stop-before). It's harder to see that's the case because I had split up this patch series,

Nonetheless, you are absolutely correct that some amount of unit testing is needed.

[czhang]

By the way, the reason why I've had a hard time with the unit tests is that MIR modules are not so easily serialized and read as IR modules are, because the representation isn't as static. Any suggestions on how to achieve something similar to parseIR cleanly without resorting to the mess in llc would be highly appreciated. For now, testing is done by checking bitwise identical outputs from both pass managers, leveraging the parsing/machine function extraction boilerplate in llc.

If there is in fact a need to use the same machine function extraction functions from MachineModuleInfoAnalysis that are only present in the next patch, we start to have coupling issues with the child dependencies of this pass, since properly handling the MIR parsing, serializing, and machine function extraction is done in the next few patches. So then there are two ways to go forward:

1. Add unit tests at the end of this patch series.
2. Leverage the legacy pass manager to do parsing and readout of machine functions from the yaml format for the unit tests.

Any particular opinions? I would be in favor of 1. so we can start to land some of these patches in the series (2/7 with lgtm) that have already been approved and depend on this one so that they don't fall prey to more rebase issues later. It is rather hard to maintain a seven patch series.

Otherwise the unit tests would mostly be a straight copy of the tests for the Module info ones in PassManagerTest.cpp, with the proper renaming done, modulo the differences in parsing and matching of serialized MIR.

[ychen]

Hello, Charlie went back to school after the internship. I'm trying the address the comments in a new patch. D67687

llvm/include/llvm/CodeGen/PassManager.h

+//===- PassManager.h ---------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// Interface for the pass manager functionality used for CodeGen.
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CODEGEN_PASS_MANAGER_H
+#define LLVM_CODEGEN_PASS_MANAGER_H
+
+#include "llvm/Analysis/OptimizationRemarkEmitter.h"
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachineModuleInfo.h"
+#include "llvm/CodeGen/MachineOptimizationRemarkEmitter.h"
+#include "llvm/IR/PassManager.h"
+#include "llvm/IR/PassManagerInternal.h"
+
+namespace llvm {
+using namespace ore;
+
+/// Convenience typedef for the MachineFunction analysis manager.
+using MachineFunctionAnalysisManager = AnalysisManager<MachineFunction>;
+/// Convenience typedef for a pass manager over functions.
+using MachineFunctionPassManager = PassManager<MachineFunction>;
+
+using MachineFunctionAnalysisManagerFunctionProxy =
+    InnerAnalysisManagerProxy<MachineFunctionAnalysisManager, Function>;
+
+using FunctionAnalysisManagerMachineFunctionProxy =
+  OuterAnalysisManagerProxy<FunctionAnalysisManager, MachineFunction>;
+
+template <typename MachineFunctionPassT>
+class FunctionToMachineFunctionPassAdaptor
+    : public PassInfoMixin<ModuleToFunctionPassAdaptor<MachineFunctionPassT>> {
+public:
+  explicit FunctionToMachineFunctionPassAdaptor(MachineFunctionPassT Pass)
+      : Pass(std::move(Pass)) {}
+
+  PreservedAnalyses run(Function &F, FunctionAnalysisManager &AM) {
+    PreservedAnalyses PA = PreservedAnalyses::all();
+
+    // Do not codegen any 'available_externally' functions at all, they have
+    // definitions outside the translation unit.
+    if (F.hasAvailableExternallyLinkage())
+      return PA;
+
+    MachineFunctionAnalysisManager &MFAM =
+        AM.getResult<MachineFunctionAnalysisManagerFunctionProxy>(F)
+            .getManager();
+
+    MachineModuleInfo *MMI =
+        AM.getResult<ModuleAnalysisManagerFunctionProxy>(F)
+            .getManager()
+            .getCachedResult<MachineModuleAnalysis>(*F.getParent());
+    MachineFunction &MF = MMI->getOrCreateMachineFunction(F);
+
+    // Collect the MI count of the function before the pass.
+    unsigned CountBefore, CountAfter;
+
+    // Check if the user asked for size remarks.
+    bool ShouldEmitSizeRemarks =
+        F.getParent()->shouldEmitInstrCountChangedRemark();
+
+    // If we want size remarks, collect the number of MachineInstrs in our
+    // MachineFunction before the pass runs.
+    if (ShouldEmitSizeRemarks)
+      CountBefore = MF.getInstructionCount();
+
+    // Request PassInstrumentation from analysis manager, will use it to run
+    // instrumenting callbacks for the passes later.
+    // TODO PASS INSTRUMENTATION
+    // PassInstrumentation PI = AM.getResult<PassInstrumentationAnalysis>(F);
+
+    // if (PI.runBeforePass<Function>(Pass, MF))
+    //   PA = Pass.run(MF, MFAM);
+
+    // PI.runAfterPass(Pass, MF);
+
+    Pass.run(MF, MFAM);
+
+    if (ShouldEmitSizeRemarks) {
+      // We wanted size remarks. Check if there was a change to the number of
+      // MachineInstrs in the module. Emit a remark if there was a change.
+      CountAfter = MF.getInstructionCount();
+      if (CountBefore != CountAfter) {
+        MachineOptimizationRemarkEmitter MORE(MF, nullptr);
+        MORE.emit([&]() {
+            int64_t Delta = static_cast<int64_t>(CountAfter) -
+              static_cast<int64_t>(CountBefore);
+            MachineOptimizationRemarkAnalysis R("size-info", "FunctionMISizeChange",
+                                                MF.getFunction().getSubprogram(),
+                                                &MF.front());
+            R << NV("Pass", getTypeName<MachineFunctionPassT>())
+              << ": Function: " << NV("Function", F.getName()) << ": "
+              << "MI Instruction count changed from "
+              << NV("MIInstrsBefore", CountBefore) << " to "
+              << NV("MIInstrsAfter", CountAfter)
+              << "; Delta: " << NV("Delta", Delta);
+            return R;
+          });
+      }
+    }
+
+    // The FunctionAnalysisManagerModuleProxy is preserved because (we assume)
+    // the function passes we ran didn't add or remove any functions.
+    //
+    // We also preserve all analyses on Machine Functions, because we did all the
+    // invalidation we needed to do above.
+    PA.preserveSet<AllAnalysesOn<MachineFunction>>();
+    PA.preserve<MachineFunctionAnalysisManagerFunctionProxy>();
+    assert(PA.areAllPreserved());
+    return PA;
+  }
+
+private:
+  MachineFunctionPassT Pass;
+};
+
+template <typename MachineFunctionPassT>
+FunctionToMachineFunctionPassAdaptor<MachineFunctionPassT>
+createFunctionToMachineFunctionPassAdaptor(MachineFunctionPassT Pass) {
+  return FunctionToMachineFunctionPassAdaptor<MachineFunctionPassT>(
+      std::move(Pass));
+}
+
+} // end namespace llvm
+
+#endif // LLVM_CODEGEN_PASS_MANAGER_H

llvm/lib/CodeGen/CMakeLists.txt

add_llvm_library(LLVMCodeGen
   MachineTraceMetrics.cpp
   MachineVerifier.cpp
   PatchableFunction.cpp
   MIRPrinter.cpp
   MIRPrintingPass.cpp
   MacroFusion.cpp
   OptimizePHIs.cpp
   ParallelCG.cpp
+  PassManager.cpp
   PeepholeOptimizer.cpp
   PHIElimination.cpp
   PHIEliminationUtils.cpp
   PostRAHazardRecognizer.cpp
   PostRASchedulerList.cpp
   PreISelIntrinsicLowering.cpp
   ProcessImplicitDefs.cpp
   PrologEpilogInserter.cpp

llvm/lib/CodeGen/PassManager.cpp

+//===-- PassManager.cpp -------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file contains the pass management machinery for machine functions.
+//
+//===----------------------------------------------------------------------===//
+
+#include "llvm/CodeGen/PassManager.h"
+
+using namespace llvm;
+
+namespace llvm {
+template class AllAnalysesOn<MachineFunction>;
+template class AnalysisManager<MachineFunction>;
+template class PassManager<MachineFunction>;
+template class InnerAnalysisManagerProxy<MachineFunctionAnalysisManager,
+                                         Function>;
+template class OuterAnalysisManagerProxy<FunctionAnalysisManager,
+                                         MachineFunction>;
+
+template <>
+bool MachineFunctionAnalysisManagerFunctionProxy::Result::invalidate(
+    Function &F, const PreservedAnalyses &PA,
+    FunctionAnalysisManager::Invalidator &Inv) {
+  // Just assume that all proxies are valid for the time being until we run into trouble.
+  // Return false to indicate that this result is still a valid proxy.
+  return false;
+}
+} // namespace llvm

llvm/tools/llc/CMakeLists.txt

set(LLVM_LINK_COMPONENTS
   MIRParser
   Remarks
   ScalarOpts
   SelectionDAG
   Support
   Target
   TransformUtils
   Vectorize
+  Passes
   )
 
 # Support plugins.
 set(LLVM_NO_DEAD_STRIP 1)
 
 add_llvm_tool(llc
   llc.cpp
 
   DEPENDS
   intrinsics_gen
   )
 export_executable_symbols(llc)

llvm/tools/llc/LLVMBuild.txt

 ;   http://llvm.org/docs/LLVMBuild.html
 ;
 ;===------------------------------------------------------------------------===;
 
 [component_0]
 type = Tool
 name = llc
 parent = Tools
-required_libraries = AsmParser BitReader IRReader MIRParser TransformUtils Scalar Vectorize all-targets
+required_libraries = AsmParser BitReader IRReader MIRParser TransformUtils Scalar Vectorize Passes all-targets