[PATCH] D67687: [NewPM][CodeGen] Introduce machine pass and machine pass manager

Thu Jul 23 16:40:57 PDT 2020

asbirlea added a comment.

In D67687#2170879 <https://reviews.llvm.org/D67687#2170879>, @ychen wrote:

> In D67687#2170494 <https://reviews.llvm.org/D67687#2170494>, @asbirlea wrote:
>
> > IIRC the MIR pipeline is essentially flat (vs the IR one), so at this point it's probably fine to not have the adaptor complexity. However, if this changes to become more than a one time use, then perhaps the Module-Function hierarchy makes sense.
> >  Let me ask this differently, if the CodegenNPM introduced the `ModuleToFunctionPassAdaptor` now, and it was only used during codegen set-up (assuming negligible overhead for this complexity), is it possible it will be useful long-term in case there are changes in the current restrictions?
>
>
> TBH, I don't think the adaptor class would be useful for cases other than codegen setup, because a partial MIR pipeline seems not very useful to be run as part of an IR pipeline. The only other place, I could think of, the adaptor could be useful is to concatenate the pre-codengen IR pipeline with the MIR pipeline. But similar to the possibility of combining opt-pipeline with codegen pipeline, I'm not sure it brings actual benefits other than aesthetics.

Thank you for clarifying. Let's move forward with the current design and document it to keep a record in case it needs to be revisited in the future. It doesn't sound like it will be the case at this point, but it's nice to have the doc to point to :-).

>> It would be very helpful if you added an overall comment explanation (or adding to the documentation) on the decision of which parts of the IR NPM infra is used in the CodegenNPM.
> 
> Sure, will do.

================
Comment at: llvm/lib/CodeGen/MachinePassManager.cpp:35
+
+  if (DebugLogging)
+    dbgs() << "Starting " << getTypeName<MachineFunction>()
----------------
arsenm wrote:
> asbirlea wrote:
> > arsenm wrote:
> > > Braces
> > I'm assuming this and other places meant remove braces for single instruction for/if?
> I meant add for multiline
Ah I see, I didn't see this in the styleguide. I always favored not having the braces for single statement if it doesn't affect readability, but either works here I guess.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67687