[PassManager] Support MachineFunctionProperties #83668

paperchalice · 2024-03-02T10:26:41Z

This pull request adds MachineFunctionProperties support. If a pass wants to modify machine function properties, it must derive from MachinePassInfoMixin and define some static methods like in legacy pass manager. A test pass RequireAllMachineFunctionPropertiesPass is also added here, which could be a example.

llvmbot · 2024-03-02T10:27:10Z

@llvm/pr-subscribers-llvm-ir

Author: None (paperchalice)

Changes

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

6 Files Affected:

(modified) llvm/include/llvm/IR/PassInstrumentation.h (+33)
(modified) llvm/include/llvm/IR/PassManager.h (+9-4)
(modified) llvm/include/llvm/IR/PassManagerInternal.h (+3)
(modified) llvm/include/llvm/Passes/StandardInstrumentations.h (+6)
(modified) llvm/lib/CodeGen/MachinePassManager.cpp (+23-2)
(modified) llvm/lib/Passes/StandardInstrumentations.cpp (+31)

diff --git a/llvm/include/llvm/IR/PassInstrumentation.h b/llvm/include/llvm/IR/PassInstrumentation.h
index 519a5e46b4373b..2ee06a51cf1df1 100644
--- a/llvm/include/llvm/IR/PassInstrumentation.h
+++ b/llvm/include/llvm/IR/PassInstrumentation.h
@@ -58,6 +58,13 @@
 
 namespace llvm {
 
+namespace detail {
+
+struct MachinePassConcept;
+
+}
+
+class MachineFunction;
 class PreservedAnalyses;
 class StringRef;
 
@@ -78,7 +85,12 @@ class PassInstrumentationCallbacks {
   using BeforePassFunc = bool(StringRef, Any);
   using BeforeSkippedPassFunc = void(StringRef, Any);
   using BeforeNonSkippedPassFunc = void(StringRef, Any);
+  using BeforeNonSkippedMachineFunctionPassFunc =
+      void(const detail::MachinePassConcept &, MachineFunction &);
   using AfterPassFunc = void(StringRef, Any, const PreservedAnalyses &);
+  using AfterMachineFunctionPassFunc = void(const detail::MachinePassConcept &,
+                                            MachineFunction &,
+                                            const PreservedAnalyses &);
   using AfterPassInvalidatedFunc = void(StringRef, const PreservedAnalyses &);
   using BeforeAnalysisFunc = void(StringRef, Any);
   using AfterAnalysisFunc = void(StringRef, Any);
@@ -107,6 +119,11 @@ class PassInstrumentationCallbacks {
     BeforeNonSkippedPassCallbacks.emplace_back(std::move(C));
   }
 
+  template <typename CallableT>
+  void registerBeforeNonSkippedMachineFunctionPassCallback(CallableT C) {
+    BeforeNonSkippedMachineFunctionPassCallbacks.emplace_back(std::move(C));
+  }
+
   template <typename CallableT>
   void registerAfterPassCallback(CallableT C, bool ToFront = false) {
     if (ToFront)
@@ -115,6 +132,11 @@ class PassInstrumentationCallbacks {
       AfterPassCallbacks.emplace_back(std::move(C));
   }
 
+  template <typename CallableT>
+  void registerAfterMachineFunctionPassCallback(CallableT C) {
+    AfterMachineFunctionPassCallbacks.emplace_back(std::move(C));
+  }
+
   template <typename CallableT>
   void registerAfterPassInvalidatedCallback(CallableT C, bool ToFront = false) {
     if (ToFront)
@@ -166,8 +188,12 @@ class PassInstrumentationCallbacks {
   /// These are run on passes that are about to be run.
   SmallVector<llvm::unique_function<BeforeNonSkippedPassFunc>, 4>
       BeforeNonSkippedPassCallbacks;
+  SmallVector<llvm::unique_function<BeforeNonSkippedMachineFunctionPassFunc>, 4>
+      BeforeNonSkippedMachineFunctionPassCallbacks;
   /// These are run on passes that have just run.
   SmallVector<llvm::unique_function<AfterPassFunc>, 4> AfterPassCallbacks;
+  SmallVector<llvm::unique_function<AfterMachineFunctionPassFunc>, 4>
+      AfterMachineFunctionPassCallbacks;
   /// These are run passes that have just run on invalidated IR.
   SmallVector<llvm::unique_function<AfterPassInvalidatedFunc>, 4>
       AfterPassInvalidatedCallbacks;
@@ -247,6 +273,9 @@ class PassInstrumentation {
     return ShouldRun;
   }
 
+  bool runBeforeMachineFunctionPass(const detail::MachinePassConcept &Pass,
+                                    MachineFunction &MF) const;
+
   /// AfterPass instrumentation point - takes \p Pass instance that has
   /// just been executed and constant reference to \p IR it operates on.
   /// \p IR is guaranteed to be valid at this point.
@@ -258,6 +287,10 @@ class PassInstrumentation {
         C(Pass.name(), llvm::Any(&IR), PA);
   }
 
+  void runAfterMachineFunctionPass(const detail::MachinePassConcept &Pass,
+                                   MachineFunction &MF,
+                                   const PreservedAnalyses &PA) const;
+
   /// AfterPassInvalidated instrumentation point - takes \p Pass instance
   /// that has just been executed. For use when IR has been invalidated
   /// by \p Pass execution.
diff --git a/llvm/include/llvm/IR/PassManager.h b/llvm/include/llvm/IR/PassManager.h
index c03d49c3b7b978..ad7a42223549e5 100644
--- a/llvm/include/llvm/IR/PassManager.h
+++ b/llvm/include/llvm/IR/PassManager.h
@@ -64,6 +64,8 @@ extern llvm::cl::opt<bool> UseNewDbgInfoFormat;
 
 namespace llvm {
 
+class MachineFunction;
+
 // RemoveDIs: Provide facilities for converting debug-info from one form to
 // another, which are no-ops for everything but modules.
 template <class IRUnitT> inline bool shouldConvertDbgInfo(IRUnitT &IR) {
@@ -271,8 +273,10 @@ class PassManager : public PassInfoMixin<
   LLVM_ATTRIBUTE_MINSIZE
       std::enable_if_t<!std::is_same<PassT, PassManager>::value>
       addPass(PassT &&Pass) {
-    using PassModelT =
-        detail::PassModel<IRUnitT, PassT, AnalysisManagerT, ExtraArgTs...>;
+    using PassModelT = std::conditional_t<
+        std::is_same_v<IRUnitT, MachineFunction>,
+        detail::MachinePassModel<PassT>,
+        detail::PassModel<IRUnitT, PassT, AnalysisManagerT, 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>(
@@ -298,8 +302,9 @@ class PassManager : public PassInfoMixin<
   static bool isRequired() { return true; }
 
 protected:
-  using PassConceptT =
-      detail::PassConcept<IRUnitT, AnalysisManagerT, ExtraArgTs...>;
+  using PassConceptT = std::conditional_t<
+      std::is_same_v<IRUnitT, MachineFunction>, detail::MachinePassConcept,
+      detail::PassConcept<IRUnitT, AnalysisManagerT, ExtraArgTs...>>;
 
   std::vector<std::unique_ptr<PassConceptT>> Passes;
 };
diff --git a/llvm/include/llvm/IR/PassManagerInternal.h b/llvm/include/llvm/IR/PassManagerInternal.h
index 4ada6ee5dd6831..06b941fe947fdb 100644
--- a/llvm/include/llvm/IR/PassManagerInternal.h
+++ b/llvm/include/llvm/IR/PassManagerInternal.h
@@ -328,6 +328,9 @@ struct AnalysisPassModel
   PassT Pass;
 };
 
+struct MachinePassConcept;
+template <typename PassT> struct MachinePassModel;
+
 } // end namespace detail
 
 } // end namespace llvm
diff --git a/llvm/include/llvm/Passes/StandardInstrumentations.h b/llvm/include/llvm/Passes/StandardInstrumentations.h
index 8c6a44876d5459..9c2ec0a8120aaf 100644
--- a/llvm/include/llvm/Passes/StandardInstrumentations.h
+++ b/llvm/include/llvm/Passes/StandardInstrumentations.h
@@ -563,6 +563,11 @@ class PrintCrashIRInstrumentation {
   static void SignalHandler(void *);
 };
 
+class MachineFunctionPropertiesInstrumentation {
+public:
+  void registerCallbacks(PassInstrumentationCallbacks &PIC, bool Verify);
+};
+
 /// This class provides an interface to register all the standard pass
 /// instrumentations and manages their state (if any).
 class StandardInstrumentations {
@@ -580,6 +585,7 @@ class StandardInstrumentations {
   PrintCrashIRInstrumentation PrintCrashIR;
   IRChangedTester ChangeTester;
   VerifyInstrumentation Verify;
+  MachineFunctionPropertiesInstrumentation MFProp;
 
   bool VerifyEach;
 
diff --git a/llvm/lib/CodeGen/MachinePassManager.cpp b/llvm/lib/CodeGen/MachinePassManager.cpp
index 9a750b5bed4339..8bc8bf259a44c2 100644
--- a/llvm/lib/CodeGen/MachinePassManager.cpp
+++ b/llvm/lib/CodeGen/MachinePassManager.cpp
@@ -119,13 +119,13 @@ PassManager<MachineFunction>::run(MachineFunction &MF,
           ->getMMI();
   PreservedAnalyses PA = PreservedAnalyses::all();
   for (auto &Pass : Passes) {
-    if (!PI.runBeforePass<MachineFunction>(*Pass, MF))
+    if (!PI.runBeforeMachineFunctionPass(*Pass, MF))
       continue;
 
     PreservedAnalyses PassPA = Pass->run(MF, MFAM);
     if (MMI.getMachineFunction(F)) {
       MFAM.invalidate(MF, PassPA);
-      PI.runAfterPass(*Pass, MF, PassPA);
+      PI.runAfterMachineFunctionPass(*Pass, MF, PassPA);
     } else {
       MFAM.clear(MF, F.getName());
       PI.runAfterPassInvalidated<MachineFunction>(*Pass, PassPA);
@@ -135,4 +135,25 @@ PassManager<MachineFunction>::run(MachineFunction &MF,
   return PA;
 }
 
+bool PassInstrumentation::runBeforeMachineFunctionPass(
+    const detail::MachinePassConcept &Pass, MachineFunction &MF) const {
+  bool ShouldRun = runBeforePass(Pass, MF);
+  if (!ShouldRun)
+    return false;
+
+  if (Callbacks)
+    for (auto &C : Callbacks->BeforeNonSkippedMachineFunctionPassCallbacks)
+      C(Pass, MF);
+  return true;
+}
+
+void PassInstrumentation::runAfterMachineFunctionPass(
+    const detail::MachinePassConcept &Pass, MachineFunction &MF,
+    const PreservedAnalyses &PA) const {
+  runAfterPass(Pass, MF, PA);
+  if (Callbacks)
+    for (auto &C : Callbacks->AfterMachineFunctionPassCallbacks)
+      C(Pass, MF, PA);
+}
+
 } // namespace llvm
diff --git a/llvm/lib/Passes/StandardInstrumentations.cpp b/llvm/lib/Passes/StandardInstrumentations.cpp
index 697988b3fc7c0b..1e5c9986172e91 100644
--- a/llvm/lib/Passes/StandardInstrumentations.cpp
+++ b/llvm/lib/Passes/StandardInstrumentations.cpp
@@ -20,6 +20,7 @@
 #include "llvm/Analysis/LazyCallGraph.h"
 #include "llvm/Analysis/LoopInfo.h"
 #include "llvm/CodeGen/MachineFunction.h"
+#include "llvm/CodeGen/MachinePassManager.h"
 #include "llvm/IR/Constants.h"
 #include "llvm/IR/Function.h"
 #include "llvm/IR/Module.h"
@@ -2415,6 +2416,35 @@ void PrintCrashIRInstrumentation::registerCallbacks(
       });
 }
 
+void MachineFunctionPropertiesInstrumentation::registerCallbacks(
+    PassInstrumentationCallbacks &PIC, bool Verify) {
+  if (Verify) {
+    PIC.registerBeforeNonSkippedMachineFunctionPassCallback(
+        [](const detail::MachinePassConcept &Pass, MachineFunction &MF) {
+          auto &MFProps = MF.getProperties();
+          auto RequiredProperties = Pass.getRequiredProperties();
+          if (!MFProps.verifyRequiredProperties(RequiredProperties)) {
+            errs() << "MachineFunctionProperties required by " << Pass.name()
+                   << " pass are not met by function " << MF.getName() << ".\n"
+                   << "Required properties: ";
+            RequiredProperties.print(errs());
+            errs() << "\nCurrent properties: ";
+            MFProps.print(errs());
+            errs() << "\n";
+            report_fatal_error("MachineFunctionProperties check failed");
+          }
+        });
+  }
+
+  PIC.registerAfterMachineFunctionPassCallback(
+      [](const detail::MachinePassConcept &Pass, MachineFunction &MF,
+         const PreservedAnalyses &) {
+        auto &MFProp = MF.getProperties();
+        MFProp.set(Pass.getClearedProperties());
+        MFProp.set(Pass.getSetProperties());
+      });
+}
+
 void StandardInstrumentations::registerCallbacks(
     PassInstrumentationCallbacks &PIC, ModuleAnalysisManager *MAM) {
   PrintIR.registerCallbacks(PIC);
@@ -2440,6 +2470,7 @@ void StandardInstrumentations::registerCallbacks(
   // AfterCallbacks by its `registerCallbacks`. This is necessary
   // to ensure that other callbacks are not included in the timings.
   TimeProfilingPasses.registerCallbacks(PIC);
+  MFProp.registerCallbacks(PIC, VerifyEach);
 }
 
 template class ChangeReporter<std::string>;

arsenm · 2024-03-04T08:18:57Z

llvm/lib/Passes/StandardInstrumentations.cpp

+            RequiredProperties.print(errs());
+            errs() << "\nCurrent properties: ";
+            MFProps.print(errs());
+            errs() << "\n";


Suggested change

errs() << "\n";

errs() << '\n';

arsenm · 2024-03-04T08:19:16Z

llvm/lib/CodeGen/MachinePassManager.cpp

+  if (Callbacks)
+    for (auto &C : Callbacks->AfterMachineFunctionPassCallbacks)
+      C(Pass, MF, PA);
+}


Suggested change

if (Callbacks)

for (auto &C : Callbacks->AfterMachineFunctionPassCallbacks)

C(Pass, MF, PA);

}

if (Callbacks) {

for (auto &C : Callbacks->AfterMachineFunctionPassCallbacks)

C(Pass, MF, PA);

}

}

arsenm · 2024-03-04T08:19:35Z

llvm/lib/CodeGen/MachinePassManager.cpp

+  if (Callbacks)
+    for (auto &C : Callbacks->BeforeNonSkippedMachineFunctionPassCallbacks)
+      C(Pass, MF);


Suggested change

if (Callbacks)

for (auto &C : Callbacks->BeforeNonSkippedMachineFunctionPassCallbacks)

C(Pass, MF);

if (Callbacks) {

for (auto &C : Callbacks->BeforeNonSkippedMachineFunctionPassCallbacks)

C(Pass, MF);

}

Just follow the coding standard:

llvm-project/llvm/docs/CodingStandards.rst

Lines 1716 to 1721 in 7af4e8b

// This should also omit braces. The `for` loop contains only a single

// statement, so it shouldn't have braces. The `if` also only contains a

// single simple statement (the `for` loop), so it also should omit braces.

if (isa<FunctionDecl>(D))

for (auto *A : D.attrs())

handleAttr(A);

arsenm · 2024-03-04T08:20:05Z

llvm/include/llvm/IR/PassInstrumentation.h

@@ -78,7 +85,12 @@ class PassInstrumentationCallbacks {
  using BeforePassFunc = bool(StringRef, Any);
  using BeforeSkippedPassFunc = void(StringRef, Any);
  using BeforeNonSkippedPassFunc = void(StringRef, Any);
+  using BeforeNonSkippedMachineFunctionPassFunc =
+      void(const detail::MachinePassConcept &, MachineFunction &);


All of these typedefs should probably move to function_ref? For now I guess it follows the pattern

All of them are wrapped in llvm::unique_function.

aeubanks · 2024-03-06T17:56:25Z

a general question, is MachineFunctionProperties only used for verification? if that's the case, I'd actually rather keep the verification in MachineFunctionPassManager::run() so pass instrumentation doesn't have to know about pass manager internals

e.g.

diff --git a/llvm/include/llvm/CodeGen/MachinePassManager.h b/llvm/include/llvm/CodeGen/MachinePassManager.h
index 7713c55661cc..c563078be836 100644
--- a/llvm/include/llvm/CodeGen/MachinePassManager.h
+++ b/llvm/include/llvm/CodeGen/MachinePassManager.h
@@ -283,6 +283,18 @@ template <>
 PreservedAnalyses
 PassManager<MachineFunction>::run(MachineFunction &,
                                   AnalysisManager<MachineFunction> &);
+template <>
+template <typename PassT>
+LLVM_ATTRIBUTE_MINSIZE
+    std::enable_if_t<!std::is_same<PassT, PassManager<MachineFunction>>::value>
+    PassManager<MachineFunction>::addPass(PassT &&Pass) {
+  using MachinePassModelT = detail::MachinePassModel<PassT>;
+  // Do not use make_unique or emplace_back, they cause too many template
+  // instantiations, causing terrible compile times.
+  Passes.push_back(std::unique_ptr<detail::MachinePassConcept>(
+      new MachinePassModelT(std::forward<PassT>(Pass))));
+}
+
 extern template class PassManager<MachineFunction>;
 
 /// Convenience typedef for a pass manager over functions.
diff --git a/llvm/lib/CodeGen/MachinePassManager.cpp b/llvm/lib/CodeGen/MachinePassManager.cpp
index 9a750b5bed43..db00f5c2aa1a 100644
--- a/llvm/lib/CodeGen/MachinePassManager.cpp
+++ b/llvm/lib/CodeGen/MachinePassManager.cpp
@@ -119,6 +119,10 @@ PassManager<MachineFunction>::run(MachineFunction &MF,
           ->getMMI();
   PreservedAnalyses PA = PreservedAnalyses::all();
   for (auto &Pass : Passes) {
+    // TODO: use in verification
+    (void)static_cast<detail::MachinePassConcept *>(Pass.get())
+        ->getClearedProperties();
+
     if (!PI.runBeforePass<MachineFunction>(*Pass, MF))
       continue;

paperchalice · 2024-03-07T01:09:32Z

Handle MachineFunctionProperties in MachineFunctionPassManager now.

aeubanks · 2024-03-07T19:24:11Z

very nice and clean, thanks!

would be good to add a test (e.g. add a MachineFunction pass that intentionally breaks some MachineFunctionProperties invariant)

cdevadas · 2024-03-08T05:28:31Z

llvm/include/llvm/IR/PassManager.h

@@ -64,6 +64,8 @@ extern llvm::cl::opt<bool> UseNewDbgInfoFormat;

 namespace llvm {

+class MachineFunction;


We shouldn't have any of the MIR machinery in the files meant for IR. My hesitation is MIR being a target-specific entity, an edge case for a specific target might require some of its properties to be exposed here. I understand we want to have a unified PM for both IR and MIR passes in the LLC pipeline. But can't we have a separate folder to accommodate designs common to IR and MIR infrastructure?

We already have some implementations in llvm/CodeGen/MachinePassManager.h. Declare MachineFunction here to make PassManager<MachineFunction> contains correct pass concept, i.e. MachinePassConcept.

I wasn't talking about this instance alone. But in general, while designing the machinery for NPM backend passes.

I agree it would be ideal to move any mention of MachineFunction out of llvm/IR, but this functionality is tricky to support in a clean way

we'd like for a machine function pass to be able to say what invariants the MIR holds (via MachineFunctionProperties) and for the pass manager infra to verify that. we could always tell every pass to verify these properties before and after running. initially I thought this would be a bunch of extra boilerplate, but actually looking at MachineFunctionPass::get[Required,Set,Cleared]Properties, maybe this is more feasible than I initially imagined. Set/Cleared aren't very common, so those passes could just manually set/clear the properties on MF.getProperties(). as for Required, currently any legacy pass that wants to ensure the incoming MachineFunction has certain properties needs to override getRequiredProperties(). it might actually be less overall code if we had a helper function to verify properties that passes would call, e.g.

diff --git a/llvm/include/llvm/CodeGen/MachinePassManager.h b/llvm/include/llvm/CodeGen/MachinePassManager.h index 7713c55661cc..669076e47634 100644 --- a/llvm/include/llvm/CodeGen/MachinePassManager.h +++ b/llvm/include/llvm/CodeGen/MachinePassManager.h @@ -44,7 +44,20 @@ using MachineFunctionAnalysisManager = AnalysisManager<MachineFunction>; /// automatically mixes in \c PassInfoMixin. template <typename DerivedT> struct MachinePassInfoMixin : public PassInfoMixin<DerivedT> { - // TODO: Add MachineFunctionProperties support. + void verifyMachineFunctionProperties(MachineFunction&MF, MachineFunctionProperties RequiredProperties) { +#ifndef NDEBUG + if (!MF.getProperties().verifyRequiredProperties(RequiredProperties)) { + errs() << "MachineFunctionProperties required by " << DerivedT::name() + << " pass are not met by function " << MF.getName() << ".\n" + << "Required properties: "; + RequiredProperties.print(errs()); + errs() << "\nCurrent properties: "; + MF.getProperties().print(errs()); + errs() << "\n"; + llvm_unreachable("MachineFunctionProperties check failed"); + } +#endif + } };

and then a pass that wants to verify would do so like

FooPass::run(MachineFunction&MF, ...) { verifyMachineFunctionProperties(MF, MachineFunctionProperties().set( MachineFunctionProperties::Property::NoVRegs)); }

@arsenm @paperchalice thoughts on this approach?

as for other approaches:

initially I was hoping we might be able to augment the return value (PreservedAnalyses) of the pass here (in fact the return value was template parameterized at some point), but we also need to verify before the pass runs, so that doesn't work

I tried templatizing PassConceptT, meaning we'd specify it in the MachineFunctionPassManager declaration, but it turned out to be harder than I thought for reasons I can't remember now

I do think the current properties system is kind of annoying. We're missing quite a few instances where the get/set/clear are missing from passes. We're also missing some defined properties that are necessary. I think forcing this into the actual pass runs would make it even more annoying.

Mostly the API is bad. We have this full set of virtual functions to implement, and then you have to construct an empty MachineFunctionProperties, and set fields in it. It would be much more convenient to have something declarative

If possible, I would prefer verify machine function properties in instrumentation (e.g. by a string map), modify properties in pass, e.g. by a Scope-Bound Resource Management object, but still need to insert boilerplate code 😕.

template<typename PassT> struct Helper { MachineFunction &MF; Helper(MachineFunction &MF): MF(MF) {} // can also verify properties here ~Helper() { /* set MachineFunctionProperties */ } }; PreservedAnalysis SomePass::run(MachineFunction &MF, MachineFunctionAnalysisManager &MFAM) { Helper<std::remove_reference_t(decltype(*this))> H(MF); ... }

It would be much more convenient to have something declarative

I still don't really understand this.

FooPass::run(MachineFunction&MF, ...) { verifyMachineFunctionProperties(MF, MachineFunctionProperties().set( MachineFunctionProperties::Property::NoVRegs)); }

is less code and more explicit than writing an extra method that also returns the same MachineFunctionProperties. Can you give an example of what you have in mind?

and then you have to construct an empty MachineFunctionProperties, and set fields in it

MachineFunctionProperties::set() is like a builder, so constructing an arbitrary MachineFunctionProperties can be expressed in one statement

I would prefer verify machine function properties in instrumentation (e.g. by a string map)

can you clarify what you mean by "by a string map"?

modify properties in pass, e.g. by a Scope-Bound Resource Management object

I agree it's nice to be able to say at the beginning of run that some MachineFunction property will be set when the pass returns since putting the verify and set MachineFunctionProperties together is nice, but also I don't think it's a huge deal to have the pass manually set MachineFunctionProperties at the end of the pass. And IMO the boilerplate you're proposing is not worth it

can you clarify what you mean by "by a string map"?

E.g. register the properties in PassInstrumentationCallbacks by creating a map

// in PIC RequiredPropertiesMap[PassT::name()] = PassT::getRequiredProperties();

and verify the properties in an instrumentation, but now I realized it is a bad choice.

FooPass::run(MachineFunction&MF, ...) {
verifyMachineFunctionProperties(MF, MachineFunctionProperties().set(
MachineFunctionProperties::Property::NoVRegs));
}

This guarantees future bugs from either forgetting to add this, or being clever with putting code before this.

MachineFunctionProperties::set() is like a builder, so constructing an arbitrary MachineFunctionProperties can be expressed in one statement

A builder here is obnoxious. These constraints are expressible as a constant bitfield, the API shouldn't be built around mutation of an object

paperchalice · 2024-03-17T11:38:49Z

Another approach to modify MachineFunctionProperties. Pass manager no longer cares about MachineFunctionProperties, properties requirements are still declarative, but pass need to define a new helper variable in run, see RequireAllMachineFunctionPropertiesPass.

llvm/test/tools/llc/new-pm/machine-function-properties.mir

cdevadas

Thanks.

arsenm · 2024-03-18T13:20:07Z

llvm/lib/Passes/PassBuilder.cpp

+    : public MachinePassInfoMixin<RequireAllMachineFunctionPropertiesPass> {
+public:
+  PreservedAnalyses run(MachineFunction &MF, MachineFunctionAnalysisManager &) {
+    PropertyChanger PC(MF);


can't we pull this out into the PassManager call sites for run, instead of inside the implementations of each of them?

If we choose not to expose the relevant details of MachineFunction to PassManager, this may be unavoidable 😕.

paperchalice · 2024-03-22T14:21:46Z

Modify the run in MachinePassModel and specialize addPass, so we can still keep the declarative style.

github-actions · 2024-03-22T23:56:51Z

✅ With the latest revision this PR passed the Python code formatter.

github-actions · 2024-03-22T23:56:51Z

✅ With the latest revision this PR passed the C/C++ code formatter.

arsenm · 2024-03-26T07:18:30Z

Modify the run in MachinePassModel and specialize addPass, so we can still keep the declarative style.

Did the new unit test miss an update?

paperchalice · 2024-03-26T08:07:38Z

Modify the run in MachinePassModel and specialize addPass, so we can still keep the declarative style.

Did the new unit test miss an update?

Oops! Forgot it , thanks!.

aeubanks

this seems ok, I don't think we'll get anything much better

sorry for the very long back and forth. I think with Chandler's vision of using concepts (basically Rust dyn traits) to model passes (as opposed to inheritance), all the C++ templates goo makes this hard to do in a nice way

this will miss cases where we directly invoke a pass without going through the MachinePass concept/model, but that should be pretty rare (impossible right now?)

just one comment

aeubanks · 2024-03-26T15:57:59Z

llvm/include/llvm/CodeGen/MachinePassManager.h

+  using MachinePassModelT = detail::MachinePassModel<PassT>;
+  // Do not use make_unique or emplace_back, they cause too many template
+  // instantiations, causing terrible compile times.
+  if constexpr (std::is_base_of_v<MachinePassInfoMixin<PassT>, PassT>) {


shouldn't this always be true?

shouldn't this always be true?

Try to use MachinePassModelT only, but InvalidateAnalysisPass<T> no longer works.

aeubanks · 2024-03-26T16:20:03Z

and can you make the description more descriptive

This reverts commit 5538853.

…7136) This reverts commit 5538853. #83668 Break some test bots.

Fix preprocessor directive.

…7137) Fix preprocessor directive.

…)" (#8…" This reverts commit 4f71329.

…)"" (#87138) Reverts #87137 It introduces ambiguous template specialization problem. Revert it.

…llvm#87137) Unfortunately GCC 9 rejects code in https://godbolt.org/z/zd9r5GM3e GCC 11 accepts this code.

…7141) Unfortunately GCC 9 rejects code in https://godbolt.org/z/zd9r5GM3e GCC 11 accepts this code.

paperchalice requested review from arsenm and aeubanks March 2, 2024 10:26

llvmbot added the llvm:ir label Mar 2, 2024

paperchalice force-pushed the NPM/CodeGen/instrument branch from 2bc7965 to 00a57c1 Compare March 2, 2024 12:49

arsenm reviewed Mar 4, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/instrument branch from 00a57c1 to f5b8989 Compare March 7, 2024 01:07

paperchalice changed the title ~~[Instrumentation] Support MachineFunctionProperties~~ [PassManager] Support MachineFunctionProperties Mar 7, 2024

paperchalice force-pushed the NPM/CodeGen/instrument branch from f5b8989 to 84bf5e2 Compare March 7, 2024 01:35

arsenm approved these changes Mar 7, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/instrument branch from 84bf5e2 to 22aa716 Compare March 7, 2024 04:37

cdevadas reviewed Mar 8, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/instrument branch from 8526669 to 67f0675 Compare March 17, 2024 11:39

arsenm reviewed Mar 18, 2024

View reviewed changes

llvm/test/tools/llc/new-pm/machine-function-properties.mir Outdated Show resolved Hide resolved

cdevadas approved these changes Mar 18, 2024

View reviewed changes

arsenm reviewed Mar 18, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/instrument branch 2 times, most recently from e149930 to ef6db83 Compare March 22, 2024 14:21

paperchalice force-pushed the NPM/CodeGen/instrument branch 2 times, most recently from 175bd87 to 8173bbf Compare March 22, 2024 23:55

paperchalice force-pushed the NPM/CodeGen/instrument branch 2 times, most recently from 5f2e04d to beae3d0 Compare March 23, 2024 02:31

[PassManager] Support MachineFunctionProperties

38d96d7

add a simple test

b345adc

paperchalice force-pushed the NPM/CodeGen/instrument branch 2 times, most recently from 1808241 to 8a20b1d Compare March 25, 2024 02:03

paperchalice force-pushed the NPM/CodeGen/instrument branch from 8a20b1d to b9b8dee Compare March 26, 2024 08:07

Another approach

4c75a70

paperchalice force-pushed the NPM/CodeGen/instrument branch from b9b8dee to 4c75a70 Compare March 26, 2024 08:10

arsenm approved these changes Mar 26, 2024

View reviewed changes

aeubanks approved these changes Mar 26, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/instrument branch from e76fd73 to 4c75a70 Compare March 29, 2024 10:50

paperchalice merged commit 5538853 into llvm:main Mar 30, 2024
7 checks passed

paperchalice added a commit to paperchalice/llvm-project that referenced this pull request Mar 30, 2024

Revert "[PassManager] Support MachineFunctionProperties (llvm#83668)"

df208e7

This reverts commit 5538853.

paperchalice mentioned this pull request Mar 30, 2024

Revert "[PassManager] Support MachineFunctionProperties (#83668)" #87136

Merged

paperchalice added a commit that referenced this pull request Mar 30, 2024

Revert "[PassManager] Support MachineFunctionProperties (#83668)" (#8…

0be26db

…7136) This reverts commit 5538853. #83668 Break some test bots.

paperchalice added a commit to paperchalice/llvm-project that referenced this pull request Mar 30, 2024

Reland "[PassManager] Support MachineFunctionProperties (llvm#83668)"

10367c1

Fix preprocessor directive.

paperchalice added a commit that referenced this pull request Mar 30, 2024

Reland "[PassManager] Support MachineFunctionProperties (#83668)" (#8…

4f71329

…7137) Fix preprocessor directive.

paperchalice added a commit that referenced this pull request Mar 30, 2024

Revert "Reland "[PassManager] Support MachineFunctionProperties (#83668…

d0c1718

…)" (#8…" This reverts commit 4f71329.

paperchalice added a commit that referenced this pull request Mar 30, 2024

Revert "Reland "[PassManager] Support MachineFunctionProperties (#83668…

e9e4ab8

…)"" (#87138) Reverts #87137 It introduces ambiguous template specialization problem. Revert it.

paperchalice added a commit that referenced this pull request Mar 30, 2024

Reland "[PassManager] Support MachineFunctionProperties (#83668)" (#8…

b361b53

…7141) Unfortunately GCC 9 rejects code in https://godbolt.org/z/zd9r5GM3e GCC 11 accepts this code.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PassManager] Support MachineFunctionProperties #83668

[PassManager] Support MachineFunctionProperties #83668

paperchalice commented Mar 2, 2024 •

edited

llvmbot commented Mar 2, 2024

arsenm Mar 4, 2024

arsenm Mar 4, 2024

arsenm Mar 4, 2024

paperchalice Mar 4, 2024

arsenm Mar 4, 2024

paperchalice Mar 4, 2024

aeubanks commented Mar 6, 2024

paperchalice commented Mar 7, 2024

aeubanks commented Mar 7, 2024

cdevadas Mar 8, 2024

paperchalice Mar 8, 2024

cdevadas Mar 8, 2024

aeubanks Mar 11, 2024

arsenm Mar 12, 2024

arsenm Mar 12, 2024

paperchalice Mar 13, 2024 •

edited

aeubanks Mar 21, 2024

paperchalice Mar 21, 2024 •

edited

arsenm Mar 21, 2024

paperchalice commented Mar 17, 2024 •

edited

cdevadas left a comment

arsenm Mar 18, 2024

paperchalice Mar 18, 2024

paperchalice commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024

github-actions bot commented Mar 22, 2024 •

edited

arsenm commented Mar 26, 2024

paperchalice commented Mar 26, 2024

aeubanks left a comment

aeubanks Mar 26, 2024

paperchalice Mar 29, 2024

aeubanks commented Mar 26, 2024

	// This should also omit braces. The `for` loop contains only a single
	// statement, so it shouldn't have braces. The `if` also only contains a
	// single simple statement (the `for` loop), so it also should omit braces.
	if (isa<FunctionDecl>(D))
	for (auto *A : D.attrs())
	handleAttr(A);

		@@ -64,6 +64,8 @@ extern llvm::cl::opt<bool> UseNewDbgInfoFormat;

		namespace llvm {

		class MachineFunction;

[PassManager] Support MachineFunctionProperties #83668

[PassManager] Support MachineFunctionProperties #83668

Conversation

paperchalice commented Mar 2, 2024 • edited

llvmbot commented Mar 2, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks commented Mar 6, 2024

paperchalice commented Mar 7, 2024

aeubanks commented Mar 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice Mar 13, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice Mar 21, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice commented Mar 17, 2024 • edited

cdevadas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paperchalice commented Mar 22, 2024 • edited

github-actions bot commented Mar 22, 2024

github-actions bot commented Mar 22, 2024 • edited

arsenm commented Mar 26, 2024

paperchalice commented Mar 26, 2024

aeubanks left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aeubanks commented Mar 26, 2024

paperchalice commented Mar 2, 2024 •

edited

paperchalice Mar 13, 2024 •

edited

paperchalice Mar 21, 2024 •

edited

paperchalice commented Mar 17, 2024 •

edited

paperchalice commented Mar 22, 2024 •

edited

github-actions bot commented Mar 22, 2024 •

edited