<div dir="ltr">I looked more closely at some of the remaining check-llvm failures under NPM, and quite a few are due to passes that haven't been ported to NPM yet. The ones I looked at all share the trait of needing some analysis to provide a TargetMachine, which doesn't exist in NPM yet. So actually some of your work is required for the optimizer pipeline NPM switch.</div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Mon, Jul 13, 2020 at 6:23 PM Chen, Yuanfang <<a href="mailto:Yuanfang.Chen@sony.com">Yuanfang.Chen@sony.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><br>
<br>
<br>
-Yuanfang<br>
<br>
> -----Original Message-----<br>
> From: Arthur Eubanks <<a href="mailto:aeubanks@google.com" target="_blank">aeubanks@google.com</a>><br>
> Sent: Monday, July 13, 2020 12:49 PM<br>
> To: Chen, Yuanfang <<a href="mailto:Yuanfang.Chen@sony.com" target="_blank">Yuanfang.Chen@sony.com</a>><br>
> Cc: LLVM Developers' List <<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>><br>
> Subject: Re: [llvm-dev] [RFC] Introducing classes for the codegen driven by<br>
> new pass manager<br>
> <br>
>       While we're still working towards using NPM for optimizer pipeline by<br>
> default, we still don't have a machine pass interface and the corresponding<br>
> machine pass manager using NPM. The potential benefits using NPM aside,<br>
> this inhibits us from making any progress on deprecating LPM for the<br>
> codegen pipeline which blocks removing LPM altogether. The purpose of this<br>
> series of patches is to (1) let pass developers write or port machine passes to<br>
> a new machine pass interface and be able to test it with `llc`. (2) let a target<br>
> have the choice of implementing the codegen pipeline using NPM (Work-in-<br>
> Progress). Maybe it is obvious already, but I also want to mention that these<br>
> patches do not intend to force a target to migrate to NPM right way.<br>
> <br>
> <br>
> Awesome!<br>
> <br>
> It would be awesome to delete all the LPM infra at some point in the future.<br>
> But even just deleting all the optimizer pipeline LPM infra would be a big win,<br>
> and that shouldn't be tied to codegen.<br>
> <br>
<br>
True. Opt and codegen pipeline could make independent progress of NPM migration. <br>
<br>
> <br>
>       * Goal-1 *<br>
>       <a href="https://reviews.llvm.org/D67687" rel="noreferrer" target="_blank">https://reviews.llvm.org/D67687</a><br>
> <br>
>       Four member methods of a machine pass are recognized by the<br>
> machine pass manager:<br>
>       (1) `PreservedAnalyses run(MachineFunction &,<br>
> MachineFunctionAnalysisManager &)`. Majority of the machine passes use<br>
> this.<br>
>       (2) `Error doInitialization(Module &,<br>
> MachineFunctionAnalysisManager &)`. Passes like AsmPrinter needs a hook<br>
> to lower/transform global constructs. (invoked before all passes `run`<br>
> method)<br>
>       (3) `Error doFinalization(Module &,<br>
> MachineFunctionAnalysisManager &)`. Client: PrintMIRPass.  This is also for<br>
> completeness. (invoked after all passes `run` method)<br>
>       (4) `Error run(Module &, MachineFunctionAnalysisManager &)`.<br>
> Client: MachineOutliner, DebugifyMachineModule. I would call this machine<br>
> module pass which needs a global scope. It is like (1) but subject to pass<br>
> ordering. Currently, a pass either define this or (1), but not both.<br>
> <br>
>       (doInitialization/doFinalization is currently not supported by the NPM<br>
> optimizer pipeline because there is no real need for it.<br>
>       <a href="http://lists.llvm.org/pipermail/llvm-dev/2018-" rel="noreferrer" target="_blank">http://lists.llvm.org/pipermail/llvm-dev/2018-</a><br>
> September/126504.html)<br>
> <br>
> <br>
> Are doInitialization/doFinalization really necessary? As mentioned in the<br>
> previous discussion, it seems like usually the things in<br>
> doInitialization/doFinalization are not logically in the right place.<br>
> For example, maybe PrintMIRPass should just be a module pass, like<br>
> PrintModulePass.<br>
<br>
Good point! It is very likely that PrintMIRPass could be implemented with (4) above.<br>
For AsmPrinter's uses of ` doInitialization`, I'm not 100% sure.  It looks like it could also be replaced by (4).<br>
But difference is ` doInitialization` is invoked before all passes `run` method, whereas (4) is invoked by pass order.<br>
I don't know if there are any implicit/subtle dependency in this regard.<br>
<br>
It would be great to have some codegen folks to shed light on it here.<br>
<br>
> <br>
> <br>
> <br>
>       * Goal-2 *<br>
>       <a href="https://reviews.llvm.org/D67687" rel="noreferrer" target="_blank">https://reviews.llvm.org/D67687</a><br>
> <br>
>       Unlike IR where `has-a` relationship exists among<br>
> module/function/loop/cgscc etc., the MIR does not have `has-a` relationship<br>
> with any kind of IR. It does have a one-on-one relationship with IR function.<br>
> So, transforming MIR does not change any IR unit at all. Invalidating a MIR<br>
> analysis result also does not invalidate any IR analysis result.<br>
> <br>
> <br>
>       Based on the above observation, the machine pass manager runs<br>
> standalone, i.e. combining it with other types of pass managers using adaptor<br>
> passes are not supported. There is also no proxy defined for machine<br>
> analysis manager with any other types of analysis managers. The machine<br>
> analysis manager does provide API to register and query IR analysis because<br>
> machine passes need IR analysis result in general.<br>
> <br>
> <br>
> Maybe this is my lack of familiarity with codegen, but why doesn't a Module<br>
> contain all its MachineFunctions? It seems like that adaptor would be<br>
> necessary.<br>
<br>
Because their data structures are separated. Logically they are representations of the same/similar thing<br>
at different abstraction level.<br>
<br>
> <br>
> <br>
>       ** Testing **<br>
>       - Since the `llc` options are compatible, as passes are ported to NPM<br>
> and various issues got resolved, we should see more tests passing when `llc -<br>
> enable-new-pm` is turned on implicitly via an (maybe) knob similar to `cmake<br>
> -DENABLE_EXPERIMENTAL_NEW_PASS_MANAGER`.<br>
>       - A buildbot to make sure no regression when `llc -enable-new-pm` is<br>
> implicitly on?<br>
>       - Any idea on this regard is much appreciated.<br>
> <br>
> <br>
> Manually running tests once in a while might be good enough, not sure if the<br>
> cost of setting up a bot that maintains some sort of list of tests that have<br>
> passed in the past is worth it. From my limited experience, tests won't really<br>
> tend to regress under NPM as long as you have some tests explicitly testing<br>
> NPM sprinkled around.<br>
<br>
Sounds good. I was afraid if it takes too long to migration codegen passes, there may be regressions introduced<br>
into IR-to-obj tests under NPM (they exercise mush more than a single or a few consecutive passes). I think it's fine to consider this in the future.<br>
<br>
<br>
<br>
</blockquote></div>