Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CodeGen] Support MachineFunctionProperties in PassConcept #79749

Closed
wants to merge 2 commits into from

Conversation

paperchalice
Copy link
Contributor

This patch extends PassConcept to support MachineFunctionProperties, MachineFunctionProperties verification work may be completed within the next few days, in new codegen instrumentations rather than pass manager.

@llvmbot
Copy link
Collaborator

llvmbot commented Jan 28, 2024

@llvm/pr-subscribers-llvm-ir

Author: None (paperchalice)

Changes

This patch extends PassConcept to support MachineFunctionProperties, MachineFunctionProperties verification work may be completed within the next few days, in new codegen instrumentations rather than pass manager.


Full diff: https://github.com/llvm/llvm-project/pull/79749.diff

6 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/MachinePassManager.h (+26-13)
  • (added) llvm/include/llvm/CodeGen/MachinePassManagerInternal.h (+68)
  • (modified) llvm/include/llvm/IR/PassManager.h (+7-2)
  • (modified) llvm/include/llvm/IR/PassManagerInternal.h (+15-2)
  • (modified) llvm/unittests/CodeGen/PassManagerTest.cpp (+4-3)
  • (modified) llvm/unittests/MIR/PassBuilderCallbacksTest.cpp (+2-2)
diff --git a/llvm/include/llvm/CodeGen/MachinePassManager.h b/llvm/include/llvm/CodeGen/MachinePassManager.h
index 662b7e8689717dc..440c3b398beaa63 100644
--- a/llvm/include/llvm/CodeGen/MachinePassManager.h
+++ b/llvm/include/llvm/CodeGen/MachinePassManager.h
@@ -25,15 +25,13 @@
 
 #include "llvm/ADT/FunctionExtras.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/CodeGen/MachinePassManagerInternal.h"
 #include "llvm/IR/PassManager.h"
 #include "llvm/Support/Error.h"
 
 #include <map>
 
 namespace llvm {
-class Module;
-class Function;
-class MachineFunction;
 
 extern template class AnalysisManager<MachineFunction>;
 
@@ -43,7 +41,28 @@ extern template class AnalysisManager<MachineFunction>;
 /// automatically mixes in \c PassInfoMixin.
 template <typename DerivedT>
 struct MachinePassInfoMixin : public PassInfoMixin<DerivedT> {
-  // TODO: Add MachineFunctionProperties support.
+  static MachineFunctionProperties getRequiredProperties() {
+    return MachineFunctionProperties();
+  }
+  static MachineFunctionProperties getSetProperties() {
+    return MachineFunctionProperties();
+  }
+  static MachineFunctionProperties getClearedProperties() {
+    return MachineFunctionProperties();
+  }
+};
+
+/// A CRTP mix-in that provides informational APIs needed for MachineFunction
+/// analysis passes. See also \c PassInfoMixin.
+template <typename DerivedT>
+struct MachineFunctionAnalysisInfoMixin
+    : public MachinePassInfoMixin<DerivedT> {
+  static AnalysisKey *ID() {
+    static_assert(
+        std::is_base_of<MachineFunctionAnalysisInfoMixin, DerivedT>::value,
+        "Must pass the derived type as the template argument!");
+    return &DerivedT::Key;
+  }
 };
 
 /// An AnalysisManager<MachineFunction> that also exposes IR analysis results.
@@ -183,9 +202,7 @@ class MachineFunctionPassManager
   template <typename PassT>
   std::enable_if_t<is_detected<has_init_t, PassT>::value>
   addDoInitialization(PassConceptT *Pass) {
-    using PassModelT =
-        detail::PassModel<MachineFunction, PassT, PreservedAnalyses,
-                          MachineFunctionAnalysisManager>;
+    using PassModelT = detail::MachinePassModel<PassT>;
     auto *P = static_cast<PassModelT *>(Pass);
     InitializationFuncs.emplace_back(
         [=](Module &M, MachineFunctionAnalysisManager &MFAM) {
@@ -205,9 +222,7 @@ class MachineFunctionPassManager
   template <typename PassT>
   std::enable_if_t<is_detected<has_fini_t, PassT>::value>
   addDoFinalization(PassConceptT *Pass) {
-    using PassModelT =
-        detail::PassModel<MachineFunction, PassT, PreservedAnalyses,
-                          MachineFunctionAnalysisManager>;
+    using PassModelT = detail::MachinePassModel<PassT>;
     auto *P = static_cast<PassModelT *>(Pass);
     FinalizationFuncs.emplace_back(
         [=](Module &M, MachineFunctionAnalysisManager &MFAM) {
@@ -236,9 +251,7 @@ class MachineFunctionPassManager
                   "machine module pass needs to define machine function pass "
                   "api. sorry.");
 
-    using PassModelT =
-        detail::PassModel<MachineFunction, PassT, PreservedAnalyses,
-                          MachineFunctionAnalysisManager>;
+    using PassModelT = detail::MachinePassModel<PassT>;
     auto *P = static_cast<PassModelT *>(Pass);
     MachineModulePasses.emplace(
         Passes.size() - 1,
diff --git a/llvm/include/llvm/CodeGen/MachinePassManagerInternal.h b/llvm/include/llvm/CodeGen/MachinePassManagerInternal.h
new file mode 100644
index 000000000000000..bbac8cb86affe9a
--- /dev/null
+++ b/llvm/include/llvm/CodeGen/MachinePassManagerInternal.h
@@ -0,0 +1,68 @@
+//===- MachinePassManagerInternal.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
+//
+//===----------------------------------------------------------------------===//
+/// \file
+///
+/// This header provides internal APIs and implementation details used by the
+/// pass management interfaces exposed in MachinePassManager.h. Most of them are
+/// copied from PassManagerInternal.h.
+/// See also PassManagerInternal.h.
+///
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CODEGEN_MACHINEPASSMANAGERINTERNAL_H
+#define LLVM_CODEGEN_MACHINEPASSMANAGERINTERNAL_H
+
+#include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/IR/PassManager.h"
+
+namespace llvm {
+
+class MachineFunctionAnalysisManager;
+
+using MachinePassConcept =
+    detail::PassConcept<MachineFunction, MachineFunctionAnalysisManager>;
+
+namespace detail {
+
+/// Template for the abstract base class used to dispatch
+/// polymorphically over pass objects. See also \c PassConcept.
+template <>
+struct PassConcept<MachineFunction, MachineFunctionAnalysisManager>
+    : public PassConceptBase<MachineFunction, MachineFunctionAnalysisManager> {
+  /// MachineFunction Properties.
+  PassConcept(MachineFunctionProperties RequiredProperties,
+              MachineFunctionProperties SetProperties,
+              MachineFunctionProperties ClearedProperties)
+      : RequiredProperties(RequiredProperties), SetProperties(SetProperties),
+        ClearedProperties(ClearedProperties) {}
+
+  MachineFunctionProperties RequiredProperties;
+  MachineFunctionProperties SetProperties;
+  MachineFunctionProperties ClearedProperties;
+};
+
+template <typename IRUnitT, typename PassT, typename PreservedAnalysesT,
+          typename AnalysisManagerT, typename... ExtraArgTs>
+template <typename MachineFunctionT, typename>
+PassModel<IRUnitT, PassT, PreservedAnalysesT, AnalysisManagerT, ExtraArgTs...>::
+    PassModel(PassT Pass, MachineFunctionProperties RequiredProperties,
+              MachineFunctionProperties SetProperties,
+              MachineFunctionProperties ClearedProperties)
+    : PassConcept<MachineFunction, MachineFunctionAnalysisManager>(
+          RequiredProperties, SetProperties, ClearedProperties),
+      Pass(std::move(Pass)) {}
+
+template <typename PassT>
+using MachinePassModel = PassModel<MachineFunction, PassT, PreservedAnalyses,
+                                   MachineFunctionAnalysisManager>;
+
+} // namespace detail
+
+} // namespace llvm
+
+#endif // LLVM_CODEGEN_MACHINEPASSMANAGERINTERNAL_H
diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h
index df548fbe9e458c4..36904bc2a784067 100644
--- a/llvm/include/llvm/IR/PassManager.h
+++ b/llvm/include/llvm/IR/PassManager.h
@@ -576,8 +576,13 @@ class PassManager : public PassInfoMixin<
                           ExtraArgTs...>;
     // Do not use make_unique or emplace_back, they cause too many template
     // instantiations, causing terrible compile times.
-    Passes.push_back(std::unique_ptr<PassConceptT>(
-        new PassModelT(std::forward<PassT>(Pass))));
+    if constexpr (std::is_same_v<IRUnitT, MachineFunction>)
+      Passes.push_back(std::unique_ptr<PassConceptT>(new PassModelT(
+          std::forward<PassT>(Pass), PassT::getRequiredProperties(),
+          PassT::getSetProperties(), PassT::getClearedProperties())));
+    else
+      Passes.push_back(std::unique_ptr<PassConceptT>(
+          new PassModelT(std::forward<PassT>(Pass))));
   }
 
   /// When adding a pass manager pass that has the same type as this pass
diff --git a/llvm/include/llvm/IR/PassManagerInternal.h b/llvm/include/llvm/IR/PassManagerInternal.h
index bcfdcb8206c45ee..8bdbdaa9dfdbd88 100644
--- a/llvm/include/llvm/IR/PassManagerInternal.h
+++ b/llvm/include/llvm/IR/PassManagerInternal.h
@@ -28,6 +28,8 @@ namespace llvm {
 template <typename IRUnitT> class AllAnalysesOn;
 template <typename IRUnitT, typename... ExtraArgTs> class AnalysisManager;
 class PreservedAnalyses;
+class MachineFunction;
+class MachineFunctionProperties;
 
 // Implementation details of the pass manager interfaces.
 namespace detail {
@@ -35,9 +37,9 @@ namespace detail {
 /// Template for the abstract base class used to dispatch
 /// polymorphically over pass objects.
 template <typename IRUnitT, typename AnalysisManagerT, typename... ExtraArgTs>
-struct PassConcept {
+struct PassConceptBase {
   // Boiler plate necessary for the container of derived classes.
-  virtual ~PassConcept() = default;
+  virtual ~PassConceptBase() = default;
 
   /// The polymorphic API which runs the pass over a given IR entity.
   ///
@@ -60,6 +62,10 @@ struct PassConcept {
   virtual bool isRequired() const = 0;
 };
 
+template <typename IRUnitT, typename AnalysisManagerT, typename... ExtraArgTs>
+struct PassConcept
+    : public PassConceptBase<IRUnitT, AnalysisManagerT, ExtraArgTs...> {};
+
 /// A template wrapper used to implement the polymorphic API.
 ///
 /// Can be instantiated for any object which provides a \c run method accepting
@@ -69,6 +75,13 @@ template <typename IRUnitT, typename PassT, typename PreservedAnalysesT,
           typename AnalysisManagerT, typename... ExtraArgTs>
 struct PassModel : PassConcept<IRUnitT, AnalysisManagerT, ExtraArgTs...> {
   explicit PassModel(PassT Pass) : Pass(std::move(Pass)) {}
+
+  template <typename MachineFunctionT = IRUnitT,
+            typename = std::enable_if_t<
+                std::is_same_v<MachineFunctionT, MachineFunction>>>
+  explicit PassModel(PassT Pass, MachineFunctionProperties RequiredProperties,
+                     MachineFunctionProperties SetProperties,
+                     MachineFunctionProperties ClearedProperties);
   // We have to explicitly define all the special member functions because MSVC
   // refuses to generate them.
   PassModel(const PassModel &Arg) : Pass(Arg.Pass) {}
diff --git a/llvm/unittests/CodeGen/PassManagerTest.cpp b/llvm/unittests/CodeGen/PassManagerTest.cpp
index 4d2c8b7bdb5f455..5e4106f286733bf 100644
--- a/llvm/unittests/CodeGen/PassManagerTest.cpp
+++ b/llvm/unittests/CodeGen/PassManagerTest.cpp
@@ -52,7 +52,7 @@ class TestFunctionAnalysis : public AnalysisInfoMixin<TestFunctionAnalysis> {
 AnalysisKey TestFunctionAnalysis::Key;
 
 class TestMachineFunctionAnalysis
-    : public AnalysisInfoMixin<TestMachineFunctionAnalysis> {
+    : public MachineFunctionAnalysisInfoMixin<TestMachineFunctionAnalysis> {
 public:
   struct Result {
     Result(int Count) : InstructionCount(Count) {}
@@ -70,7 +70,7 @@ class TestMachineFunctionAnalysis
   }
 
 private:
-  friend AnalysisInfoMixin<TestMachineFunctionAnalysis>;
+  friend MachineFunctionAnalysisInfoMixin<TestMachineFunctionAnalysis>;
   static AnalysisKey Key;
 };
 
@@ -79,7 +79,8 @@ AnalysisKey TestMachineFunctionAnalysis::Key;
 const std::string DoInitErrMsg = "doInitialization failed";
 const std::string DoFinalErrMsg = "doFinalization failed";
 
-struct TestMachineFunctionPass : public PassInfoMixin<TestMachineFunctionPass> {
+struct TestMachineFunctionPass
+    : public MachinePassInfoMixin<TestMachineFunctionPass> {
   TestMachineFunctionPass(int &Count, std::vector<int> &BeforeInitialization,
                           std::vector<int> &BeforeFinalization,
                           std::vector<int> &MachineFunctionPassCount)
diff --git a/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp b/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp
index 88522d45bc6bfa9..e0bd93aafddd8bc 100644
--- a/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp
+++ b/llvm/unittests/MIR/PassBuilderCallbacksTest.cpp
@@ -174,8 +174,8 @@ struct MockPassInstrumentationCallbacks {
 
 template <typename DerivedT> class MockAnalysisHandleBase {
 public:
-  class Analysis : public AnalysisInfoMixin<Analysis> {
-    friend AnalysisInfoMixin<Analysis>;
+  class Analysis : public MachineFunctionAnalysisInfoMixin<Analysis> {
+    friend MachineFunctionAnalysisInfoMixin<Analysis>;
     friend MockAnalysisHandleBase;
     static AnalysisKey Key;
 

@paperchalice paperchalice force-pushed the NPM/CodeGen/pm branch 2 times, most recently from b4dd3df to 3403ede Compare January 28, 2024 14:40
@paperchalice paperchalice marked this pull request as draft January 28, 2024 14:57
@paperchalice paperchalice force-pushed the NPM/CodeGen/pm branch 3 times, most recently from 8dd069b to 93f68c4 Compare January 29, 2024 04:58
@paperchalice paperchalice marked this pull request as ready for review January 29, 2024 06:20
@@ -576,8 +576,13 @@ class PassManager : public PassInfoMixin<
ExtraArgTs...>;
// Do not use make_unique or emplace_back, they cause too many template
// instantiations, causing terrible compile times.
Passes.push_back(std::unique_ptr<PassConceptT>(
new PassModelT(std::forward<PassT>(Pass))));
if constexpr (std::is_same_v<IRUnitT, MachineFunction>)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Braces

llvm/include/llvm/IR/PassManager.h Show resolved Hide resolved
@paperchalice
Copy link
Contributor Author

Address comments.

}
static MachineFunctionProperties getClearedProperties() {
return MachineFunctionProperties();
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Merge them into PassInfoMixin seems better but impossible (at least in C++17).

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure why PassManager and its surrounding infra is even in the IR library instead of Passes. we should try moving it (not necessarily right now)

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delayed review

this is kinda hairy just to make verification work (verification is obviously important, but it's still just one thing). I'm trying to think if there's a cleaner way that doesn't involve touching all the core classes

@@ -170,6 +192,17 @@ class MachineFunctionPassManager
addRunOnModule<PassT>(P);
}

// Avoid diamond problem.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you clarify that we'd be getting PassInfoMixin twice if we directly inherited from it (PassManager also inheriting from it)

/// A CRTP mix-in that provides informational APIs needed for MachineFunction
/// analysis passes. See also \c PassInfoMixin.
template <typename DerivedT>
struct MachineFunctionAnalysisInfoMixin
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not super familiar with codegen verification. I vaguely understand that there are different parts of the pipeline that require different types of verification. but do machine function analyses also need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

May not, but some MachineFunction passes like StackMapLivenessAnalysis analyses and modifies machine function.

@aeubanks
Copy link
Contributor

actually I'm not understanding why this is necessary. it looks like MachineVerifier::verify mostly reads from MachineFunction::getProperties() to determine what to verify? it does get a couple of analyses, but so do some new PM instrumentation validation passes like in PreservedCFGCheckerInstrumentation::registerCallbacks()

@paperchalice
Copy link
Contributor Author

paperchalice commented Feb 1, 2024

actually I'm not understanding why this is necessary. it looks like MachineVerifier::verify mostly reads from MachineFunction::getProperties() to determine what to verify? it does get a couple of analyses, but so do some new PM instrumentation validation passes like in PreservedCFGCheckerInstrumentation::registerCallbacks()

The relevant code is in MachineFunctionPass.cpp, the runOnFunction modifies and verifies MachineFunctionProperties. Plan to move them to CodeGen-specific instrumentation, which is not no-op.

@aeubanks
Copy link
Contributor

aeubanks commented Feb 1, 2024

oh I see, there are two parts to machine verification. one is checking that the MF has certain properties a pass requires before running the pass. two is checking that each property a MF claims it has actually holds in the instructions

Copy link
Contributor

@aeubanks aeubanks left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

will try to wrap my head around the new inheritance structure tomorrow

@@ -1292,9 +1298,37 @@ struct RequireAnalysisPass
OS << "require<" << PassName << '>';
}
static bool isRequired() { return true; }

// MachineFunctionPass interface, define Define these separately to prevent
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

extra "Define", ditto below

return MachineFunctionPropertiesT();
}

template <typename MachineFunctionPropertiesT = MachineFunctionProperties>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, are these templates just to work around the IR -> CodeGen depenency? that's not great

let me try some things locally, like making a completely separate MachineFunctionPassManager

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I see that PassModel in PassManagerInternal.h also does this...

@aeubanks
Copy link
Contributor

aeubanks commented Feb 2, 2024

I'm still looking at this but I think 100fc8c goes along a better path of separating out module/function passes, matching the rest of the new pass manager infra. Let me spend some more time on it.

This might take a couple of days. In the meantime I think it'd be valuable to figure out a couple of things:

  • which backend is the simplest that we could get working end to end (fewest number of required passes)
    • it would really be nice to get something end to end working so we can redesign things now if we hit issues before adding too many extra features
  • which passes use doInitialization/doFinalization, and is it possible to get rid of them by moving them into dedicated module passes (like you did for MIRPrinter)
    • AsmPrinter is probably the worst, we might need to split it into a pre-function module printer pass, function printer pass, and post-function module printer pass. if it's too ugly to split up, we might not be able to get rid of doInitialization/doFinalization

@paperchalice
Copy link
Contributor Author

I'm still looking at this but I think 100fc8c goes along a better path of separating out module/function passes, matching the rest of the new pass manager infra. Let me spend some more time on it.

This might take a couple of days. In the meantime I think it'd be valuable to figure out a couple of things:

  • which backend is the simplest that we could get working end to end (fewest number of required passes)

    • it would really be nice to get something end to end working so we can redesign things now if we hit issues before adding too many extra features

BPF might be a good choice, it is simple enough, not an experimental target, and has initial GlobalISel support.

  • which passes use doInitialization/doFinalization, and is it possible to get rid of them by moving them into dedicated module passes (like you did for MIRPrinter)

In legacy pass manager, all MachineFunctionPass use doInitialization to initialize the MachineFunctionProperties (see MachineFunctionPass.h)
Some machine function passes may need to create SampleProfile in doInitialization, other doInitialization/doFinalizations are simple.

  • AsmPrinter is probably the worst, we might need to split it into a pre-function module printer pass, function printer pass, and post-function module printer pass. if it's too ugly to split up, we might not be able to get rid of doInitialization/doFinalization

The original AsmPrinter is too complicate to be a pass in new pass manager, it also a polymorphic class used by many other codegen infrastructures.

@aeubanks
Copy link
Contributor

aeubanks commented Feb 2, 2024

I'm still looking at this but I think 100fc8c goes along a better path of separating out module/function passes, matching the rest of the new pass manager infra. Let me spend some more time on it.
This might take a couple of days. In the meantime I think it'd be valuable to figure out a couple of things:

  • which backend is the simplest that we could get working end to end (fewest number of required passes)

    • it would really be nice to get something end to end working so we can redesign things now if we hit issues before adding too many extra features

BPF might be a good choice, it is simple enough, not an experimental target, and has initial GlobalISel support.

sounds good

  • which passes use doInitialization/doFinalization, and is it possible to get rid of them by moving them into dedicated module passes (like you did for MIRPrinter)

In legacy pass manager, all MachineFunctionPass use doInitialization to initialize the MachineFunctionProperties (see MachineFunctionPass.h) Some machine function passes may need to create SampleProfile in doInitialization, other doInitialization/doFinalizations are simple.

for MachineFunctionProperties we can handle that specially in the MachineFunctionPassManager. I've come around to the fact that this probably can't be easily done in pass instrumentation

for SampleProfile, I see X86InsertPrefetch, is that what you're referring to? for that, we can lazily initialize the profile since it doesn't modify the IR. the case we'd need to worry about is when initialization/finalization modifies the IR because if it's touching something outside of runOnFunction as a function pass, that's bad

  • AsmPrinter is probably the worst, we might need to split it into a pre-function module printer pass, function printer pass, and post-function module printer pass. if it's too ugly to split up, we might not be able to get rid of doInitialization/doFinalization

The original AsmPrinter is too complicate to be a pass in new pass manager, it also a polymorphic class used by many other codegen infrastructures.

I'm not understanding your point, can you clarify why it's too complicated to be a new pass manager pass?

@paperchalice
Copy link
Contributor Author

100fc8c

Just an obvious fact. We need to factor out the polymorphic part of AsmPrinter to ensure AsmPrinterPass has value semantics.

@paperchalice
Copy link
Contributor Author

Replaced by #81068.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants