[PATCH] D83608: [NewPM][CodeGen] Introduce CodeGenPassBuilder to help build codegen pipeline

[Yuanfang Chen via Phabricator via llvm-commits]

ychen marked an inline comment as done.
ychen added inline comments.


================
Comment at: llvm/include/llvm/CodeGen/CodeGenPassBuilder.h:13-14
+///
+/// TODO: handle addRequiredID where, in legacy PM, one pass require other pass
+///       to run as prerequisite.
+///
----------------
aeubanks wrote:
> I believe this is explicitly not supported in the new PM
I'll remove this TODO. It was an anchor to remind myself about an existing feature. Under NPM, one target could manually add the required pass into the pipeline.


================
Comment at: llvm/include/llvm/CodeGen/MachinePassRegistry.def:38-47
+FUNCTION_PASS("mergeicmps", MergeICmpsPass, ())
+FUNCTION_PASS("lower-constant-intrinsics", LowerConstantIntrinsicsPass, ())
+FUNCTION_PASS("unreachableblockelim", UnreachableBlockElimPass, ())
+FUNCTION_PASS("consthoist", ConstantHoistingPass, ())
+FUNCTION_PASS("partially-inline-libcalls", PartiallyInlineLibCallsPass, ())
+FUNCTION_PASS("ee-instrument", EntryExitInstrumenterPass, (false))
+FUNCTION_PASS("post-inline-ee-instrument", EntryExitInstrumenterPass, (true))
----------------
aeubanks wrote:
> aeubanks wrote:
> > Is the intention that codegen IR passes will exist in both PassRegistry.def and MachinePassRegistry.def? Since people still generally use `opt` to test those passes.
> Oh I finally noticed the `AddIRPasses`. Maybe using a `PassBuilder` with backends doing something similar to `registerPassBuilderCallbacks` would require less duplicated code to parse user pipelines? Not sure if we'd really need the assert that module passes must come before function passes.
> Is the intention that codegen IR passes will exist in both PassRegistry.def and MachinePassRegistry.def? Since people still generally use opt to test those passes.

Yes, that was the intention.

> Oh I finally noticed the AddIRPasses. Maybe using a PassBuilder with backends doing something similar to registerPassBuilderCallbacks would require less duplicated code to parse user pipelines? 

`AddIRPasses` is for targets to add their own pre-codegen IR pipeline which uses a different/new IR pass manager than the optimizer IR pipeline.  The `user pipelines` parsing part for testing would be in the following patches. 

> Not sure if we'd really need the assert that module passes must come before function passes.

It is not required but a nice thing to have with a little bit of complicity only in the `class AddIRPass`. It makes function passes have better localities. 


================
Comment at: llvm/lib/CodeGen/CodeGenPassBuilder.cpp:23
+#define DUMMY_MACHINE_FUNCTION_PASS(NAME, PASS_NAME, CONSTRUCTOR)              \
+  AnalysisKey PASS_NAME::Key;
+#include "llvm/CodeGen/MachinePassRegistry.def"
----------------
aeubanks wrote:
> I assume this is to support one pass depending on another? As mentioned before, I believe the new PM explicitly doesn't support that (I can dig up a video where Chandler said that was a terrible idea if you want)
This is a straightforward way to implement `TargetPassConfig::insertPass`. The alternative is to use PassRegsitery.def pass string name to uniquely identify a pass however I'd prefer the pass string name only for debugging tools. Or we could look into moving targets away from using `TargetPassConfig::insertPass` if it is possible. With that said, we could keep this design, move over the related tests, and then reconsider other choices later since this is only involving changes to `class AddMachinePass`.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83608/new/

https://reviews.llvm.org/D83608