[CodeGen] Let `PassBuilder` support machine passes #76320

paperchalice · 2023-12-24T07:27:53Z

PassBuilder would be a better place to parse MIR pipeline. We can reuse the code to support parsing pass with parameters and targets can reuse registerPassBuilderCallbacks to register the target specific passes. PassBuilder also has ability to check whether a Pass is a machine pass, so it can replace part of the work of LLVMTargetMachine::getPassNameFromLegacyName.

paperchalice · 2023-12-24T08:16:30Z

~~In fact, PassBuilder would also be a better place to handle --start-after etc.~~ Should we distinguish between the optimization passes and codegen passes?

github-actions · 2023-12-25T04:13:32Z

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

paperchalice · 2023-12-26T12:23:05Z

llvm/unittests/MIR/PassBuilderCallbacksTest.cpp

+      }
+    };
+
+    Result run(MachineFunction &IR, MachineFunctionAnalysisManager::Base &AM) {


Not sure why MachineFunctionAnalysisManager is designed this way. An InnerAnalysisManagerProxy<IRUnit, MachineFunction> seems also OK.

hmm yeah MachineFunctionAnalysisManager does not fit in with the other analysis managers

it should be an alias for AnalysisManager<MachineFunction>, and then we should have a proxy for module and function analyses (correct me if I'm misunderstanding your question)

After looking at the code, I seem to understand the reason for this, MachineFunctionPassManager need get result from MachineModuleAnalysis which is a module analysis.

is there any specific reason it can't follow the rest of the new pass manager design and use proxies?

MachineFunctionPassManager can run on module directly (see the runin MachinePassManager), which seems a contrary to the pass manager's design.

paperchalice · 2024-01-03T10:09:09Z

Ping @aeubanks @arsenm.

aeubanks · 2024-01-08T22:22:16Z

at a high level, I think it makes sense to consolidate handling machine passes into PassBuilder

however, it's unfortunate that this cements the Passes -> CodeGen dependency. it would be great if people could use PassBuilder to only run IR passes without pulling in all of CodeGen. but if MachineFunction is part of CodeGen, we can't really avoid that dependency

paperchalice · 2024-01-09T01:19:05Z

at a high level, I think it makes sense to consolidate handling machine passes into PassBuilder

however, it's unfortunate that this cements the Passes -> CodeGen dependency. it would be great if people could use PassBuilder to only run IR passes without pulling in all of CodeGen. but if MachineFunction is part of CodeGen, we can't really avoid that dependency

Unfortunately, some IR pass and IR pass option tests need to verify the result in machine code form, an example is global-merge in AArch64 backend.

aeubanks

lg with nits

aeubanks · 2024-01-10T23:05:49Z

llvm/lib/Passes/PassBuilder.cpp

@@ -73,6 +73,7 @@
 #include "llvm/Analysis/TypeBasedAliasAnalysis.h"
 #include "llvm/Analysis/UniformityAnalysis.h"
 #include "llvm/CodeGen/CallBrPrepare.h"
+#include "llvm/CodeGen/CodeGenPassBuilder.h"


is this include necessary?

All dummy passes (DUMMY_*_PASS in MachinePassRegistry.def) are declared in this header.

oh man I don't think having those DUMMY_*_PASS was a great idea. we should just add passes to the pipeline as we port them. we really shouldn't have to include CodeGenPassBuilder.h from PassBuilder

can you just not #define DUMMY_*_PASS below?

Sure, will remove them later.

aeubanks · 2024-01-10T23:12:55Z

llvm/include/llvm/Target/TargetMachine.h

@@ -463,8 +463,7 @@ class LLVMTargetMachine : public TargetMachine {
  }

  virtual std::pair<StringRef, bool> getPassNameFromLegacyName(StringRef) {
-    llvm_unreachable(
-        "getPassNameFromLegacyName parseMIRPipeline is not overridden");
+    llvm_unreachable("getPassNameFromLegacyName is not overridden");


please submit this change separately (no review required)

aeubanks · 2024-01-10T23:13:43Z

llvm/unittests/MIR/PassBuilderCallbacksTest.cpp

@@ -0,0 +1,499 @@
+//===- unittests/MIR/PassBuilderCallbacksTest.cpp - PB Callback Tests --===//


ultra nit: line this up

paperchalice · 2024-01-11T06:38:48Z

Address the comments above.

`PassBuilder` would be a better place to parse MIR pipeline. We can reuse the code to support parsing pass with parameters and targets can reuse `registerPassBuilderCallbacks` to register the target specific passes. `PassBuilder` also has ability to check whether a Pass is a machine pass, so it can replace part of the work of `LLVMTargetMachine::getPassNameFromLegacyName`.

paperchalice · 2024-01-12T00:43:20Z

Remove CodeGenPassBuilder.h and DUMMY_*_PASS, then no machine passes are included now, because currently no machine pass uses MACHINE_*_PASS.

aeubanks

lg as is, the analysis manager stuff can be fixed later on as the current design is already in tree

paperchalice · 2024-01-12T05:46:47Z

Wrap TM with std::unique_ptr to avoid sanitizer error.

`PassBuilder` would be a better place to parse MIR pipeline. We can reuse the code to support parsing pass with parameters and targets can reuse `registerPassBuilderCallbacks` to register the target specific passes. `PassBuilder` also has ability to check whether a Pass is a machine pass.

optimisan · 2024-10-24T09:24:01Z

llvm/lib/Passes/PassBuilder.cpp

+#define MACHINE_MODULE_PASS(NAME, PASS_NAME, CONSTRUCTOR)                      \
+  if (Name == NAME) {                                                          \
+    MFPM.addPass(PASS_NAME());                                                 \
+    return Error::success();                                                   \
+  }


What does a machine module pass mean? If it's to mean module pass but for codegen, isn't MFPM.addPass() wrong as it assumes the MACHINE_MODULE_PASS pass to be a MachineFunction pass?

@paperchalice

I think it's just a placeholder as there are no "MachineModulePasses"

Oops the code is wrong here now. Initially when I took over this part, the original designer want to use machine function pass manager to manage both machine function pass and "machine module pass", but now I realized "machine module pass" is just a special kind of module pass that modifies all MachineFunctionAnalysis::Result i.e. MachineFunciton...

I didn't get what modifies all MachineFunctionAnalysis::Result mean? I see that MachineModuleAnalysis and PassInstrumentationAnalysis are in this category.

I didn't get what modifies all MachineFunctionAnalysis::Result mean? I see that MachineModuleAnalysis and PassInstrumentationAnalysis are in this category.

Code is here: MachineFunctionAnalysis. It is just a simple wrapper of MachineFunction (may convert MachineFunction to a real analysis result in future). The owner of machine functions in new pass manager is FunctionAnalysisManager rather than MachineModuleInfo in legacy pass manager.

Machine module pass can manipulate all machine functions via FunctionAnalysisManager. For example machine outliner will create some dummy IR functions and corresponding machine functions, then modify machine functions to compress the module size.

I see, yes. To summarize, Machine module passes are Module passes that modify MachineFunctions via MAM.getResult<Function...Proxy>(M)..getResult<MachineFunctionAnalysis>(F).

paperchalice requested review from arsenm and aeubanks December 24, 2023 07:27

paperchalice force-pushed the NPM/CodeGen/pass-builder branch from b889cea to c0bfd9c Compare December 24, 2023 07:57

paperchalice force-pushed the NPM/CodeGen/pass-builder branch 3 times, most recently from 461fd04 to 5fdf4e8 Compare December 25, 2023 04:10

paperchalice force-pushed the NPM/CodeGen/pass-builder branch from 5fdf4e8 to 5890047 Compare December 25, 2023 04:22

paperchalice commented Dec 26, 2023

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/pass-builder branch 2 times, most recently from e29a493 to e64daf6 Compare December 31, 2023 10:07

paperchalice mentioned this pull request Jan 3, 2024

[CodeGen] Support start/stop in CodeGenPassBuilder #70912

Merged

paperchalice force-pushed the NPM/CodeGen/pass-builder branch 2 times, most recently from 22e44a8 to c8c0931 Compare January 8, 2024 06:19

paperchalice force-pushed the NPM/CodeGen/pass-builder branch from c8c0931 to 0a6fe74 Compare January 9, 2024 03:34

arsenm requested a review from yashssh January 9, 2024 06:46

aeubanks approved these changes Jan 11, 2024

View reviewed changes

paperchalice force-pushed the NPM/CodeGen/pass-builder branch 2 times, most recently from c465a51 to 02c92b6 Compare January 11, 2024 06:38

paperchalice mentioned this pull request Jan 11, 2024

[NewPM][CodeGen] add TargetPassConfig like API #70906

Open

pravinjagtap self-requested a review January 11, 2024 12:52

paperchalice force-pushed the NPM/CodeGen/pass-builder branch from 02c92b6 to f8bd622 Compare January 12, 2024 00:36

aeubanks approved these changes Jan 12, 2024

View reviewed changes

Wrap TargetMachine by std::unique_ptr

c74a50f

paperchalice force-pushed the NPM/CodeGen/pass-builder branch from 02e17c2 to c74a50f Compare January 12, 2024 04:53

arsenm approved these changes Jan 12, 2024

View reviewed changes

paperchalice merged commit 8566cd6 into llvm:main Jan 13, 2024

paperchalice deleted the NPM/CodeGen/pass-builder branch January 13, 2024 11:40

Kamator mentioned this pull request Jan 19, 2024

Callbacks to the CodeGen Pipeline via the New Pass Manager #78677

Open

optimisan reviewed Oct 24, 2024

View reviewed changes

optimisan mentioned this pull request Oct 24, 2024

[CodeGen] Port ShadowStackGCLowering to new pass manager #75324

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[CodeGen] Let `PassBuilder` support machine passes #76320

[CodeGen] Let `PassBuilder` support machine passes #76320

paperchalice commented Dec 24, 2023

paperchalice commented Dec 24, 2023 •

edited

Loading

github-actions bot commented Dec 25, 2023 •

edited

Loading

paperchalice Dec 26, 2023

aeubanks Jan 11, 2024

paperchalice Jan 12, 2024

aeubanks Jan 12, 2024

paperchalice Jan 12, 2024

paperchalice commented Jan 3, 2024

aeubanks commented Jan 8, 2024

paperchalice commented Jan 9, 2024

aeubanks left a comment

aeubanks Jan 10, 2024

paperchalice Jan 11, 2024

aeubanks Jan 11, 2024 •

edited

Loading

paperchalice Jan 11, 2024

aeubanks Jan 10, 2024

aeubanks Jan 10, 2024

paperchalice commented Jan 11, 2024

paperchalice commented Jan 12, 2024

aeubanks left a comment

paperchalice commented Jan 12, 2024

optimisan Oct 24, 2024

yashssh Oct 24, 2024

paperchalice Oct 24, 2024

optimisan Oct 24, 2024

paperchalice Oct 24, 2024 •

edited

Loading

optimisan Oct 25, 2024

		@@ -0,0 +1,499 @@
		//===- unittests/MIR/PassBuilderCallbacksTest.cpp - PB Callback Tests --===//

[CodeGen] Let PassBuilder support machine passes #76320

[CodeGen] Let PassBuilder support machine passes #76320

Conversation

paperchalice commented Dec 24, 2023

paperchalice commented Dec 24, 2023 • edited Loading

github-actions bot commented Dec 25, 2023 • edited Loading

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 commented Jan 3, 2024

aeubanks commented Jan 8, 2024

paperchalice commented Jan 9, 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 Jan 11, 2024 • edited Loading

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 commented Jan 11, 2024

paperchalice commented Jan 12, 2024

aeubanks left a comment

Choose a reason for hiding this comment

paperchalice commented Jan 12, 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

paperchalice Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

[CodeGen] Let `PassBuilder` support machine passes #76320

[CodeGen] Let `PassBuilder` support machine passes #76320

paperchalice commented Dec 24, 2023 •

edited

Loading

github-actions bot commented Dec 25, 2023 •

edited

Loading

aeubanks Jan 11, 2024 •

edited

Loading

paperchalice Oct 24, 2024 •

edited

Loading