[llvm-dev] How to use a custom InlineAdvisor with the new pass manager

Mircea Trofin via llvm-dev llvm-dev at lists.llvm.org
Thu May 20 12:43:51 PDT 2021 

Yup, happy to help with the review!

On Thu, May 20, 2021 at 11:49 AM Neil Henning <neil.henning at unity3d.com>
wrote:

> Ok cool! I think that'd work for us. I'll put together a PR and add you as
> reviewers if that's ok!
>
> On Thu, May 20, 2021 at 7:32 PM Mircea Trofin <mtrofin at google.com> wrote:
>
>>
>>
>> On Thu, May 20, 2021 at 11:02 AM David Blaikie <dblaikie at gmail.com>
>> wrote:
>>
>>> Ah, is this more a NPM/LPM inliner design issue more broadly then -
>>> looks like the NPM inliner didn't have a virtual "getInlineCost" even prior
>>> to the inline advisor work, yeah? It has InlineParams, which then has
>>> "getInlineCost", but it's not virtual, etc.
>>>
>>> But, yes, adding this functionality (for 3rd party code to reuse the
>>> inliner with custom heuristics) does seem reasonable, and the inline
>>> advisor seems like a reasonable abstraction to use - hopefully there's an
>>> easy/reasonable enough way to add a custom InlineAdvisor (maybe that's more
>>> abstractions than needed? Not sure).
>>>
>>> Yeah, sounds like Mircea's latest reply where you could provide a
>>> callback to build the InlineAdvisor (I assume that's what Mircea meant,
>>> rather than "InlineAnalysis") -
>>>
>> Yup, that's what I meant :) thanks!
>>
>>
>>> if the advisor can't be built ahead of time (looks like it needs the
>>> specific module, etc) and handed to the InlineAdvisorAnalysis, then a
>>> functor to defer the creation until the parameters are available makes
>>> sense to me.
>>>
>>> - Dave
>>>
>>> On Thu, May 20, 2021 at 9:09 AM Neil Henning via llvm-dev <
>>> llvm-dev at lists.llvm.org> wrote:
>>>
>>>> So for us we don't use the passes from PassBuilder - we've got our own
>>>> cut down set to improve compile time by 2.5x over the default pipeline,
>>>> while still maintaining vectorization and all the other fun things
>>>> (tangentially - I was going to talk about this at EuroLLVM then COVID
>>>> killed that!).
>>>>
>>>> At the moment what I've hacked in locally is:
>>>>
>>>> struct InlinerPass final : llvm::PassInfoMixin<InlinerPass>
>>>> {
>>>>     explicit InlinerPass(llvm::InlineParams&& params) : params(params),
>>>> pass()
>>>>     {
>>>>     }
>>>>
>>>>     llvm::PreservedAnalyses run(llvm::LazyCallGraph::SCC&,
>>>> llvm::CGSCCAnalysisManager&, llvm::LazyCallGraph&,
>>>> llvm::CGSCCUpdateResult&);
>>>>
>>>> private:
>>>>     const llvm::InlineParams params;
>>>>     llvm::InlinerPass pass;
>>>> };
>>>>
>>>> llvm::PreservedAnalyses InlinerPass::run(llvm::LazyCallGraph::SCC& scc,
>>>> llvm::CGSCCAnalysisManager& analysisManager, llvm::LazyCallGraph&
>>>> callGraph, llvm::CGSCCUpdateResult& updater)
>>>> {
>>>>     llvm::Module& module = *scc.begin()->getFunction().getParent();
>>>>     llvm::FunctionAnalysisManager& functionAnalysisManager =
>>>> analysisManager.getResult<llvm::FunctionAnalysisManagerCGSCCProxy>(scc,
>>>> callGraph).getManager();
>>>>
>>>>     // Do something really awful - `OwnedAdvisor` is private in the
>>>> `llvm::InlinerPass`, but we need to set it to our own advisor. Cast the
>>>> class to a struct
>>>>     // that happens to have the same layout, and set the field that
>>>> way. This is totally undefined behaviour and bad, but LLVM hasn't given us
>>>> the tools with
>>>>     // the new pass manager to do this properly :'(.
>>>>
>>>> reinterpret_cast<LLVMInlinerPassPrivateMemberGetterHackeroo*>(&pass)->OwnedAdvisor.reset(new
>>>> BurstInlineAdvisor(module, functionAnalysisManager, params));
>>>>
>>>>     return pass.run(scc, analysisManager, callGraph, updater);
>>>> }
>>>>
>>>> So we can use the default llvm::InlinerPass but with our own custom
>>>> InlineAdvisor. I *know* this is a hack though, and I think the correct
>>>> solution (but please correct me if I'm wrong!) would be to add a version of
>>>> InlineAdvisorAnalysis that takes our own InlineAdvisor?
>>>>
>>>> With the old pass manager the Inliner just had a virtual getInlineCost
>>>> method that we could extend and override, so we didn't have this problem
>>>> there.
>>>>
>>>> On Thu, May 20, 2021 at 4:45 PM Mircea Trofin <mtrofin at google.com>
>>>> wrote:
>>>>
>>>>>
>>>>>
>>>>> On Thu, May 20, 2021 at 8:14 AM Neil Henning <neil.henning at unity3d.com>
>>>>> wrote:
>>>>>
>>>>>> I can't edit InlineAdvisorAnalysis::Result::tryCreate to add my own
>>>>>> InlineAdvisor there is the issue.
>>>>>>
>>>>> Not sure I follow: ReplayInlineAdvisor is constructed
>>>>> in InlinerPass::getAdvisor. That aside, on the comment about editing: I
>>>>> think I'm still missing some aspects of your scenario: wouldn't your
>>>>> advisor be part of llvm?
>>>>>
>>>>>>
>>>>>> I guess the easiest thing for our use-case might be to add another
>>>>>> constructor to InlineAdvisorAnalysis that takes an InlineAdvisor, and this
>>>>>> would set the Advisor field. That way I could just add to our
>>>>>> AnalysisManager our own InlineAdvisorAnalysis, and we'd get the expected
>>>>>> behaviour.
>>>>>>
>>>>> Oh - your scenario involves an advisor implemented outside the llvm
>>>>> tree, is that the case? Can you share more details, e.g. how would it be
>>>>> loaded; do you use the optimization pipelines and analysis managers in
>>>>> PassBuilder.cpp, or you'd set up your own? By better understanding the
>>>>> scenario, we can come up with a design that can probably help others, too.
>>>>>
>>>>> Thanks!
>>>>>
>>>>>
>>>>>>
>>>>>> Seems like a small enough thing to do in a PR - is that an acceptable
>>>>>> change you think?
>>>>>>
>>>>>> On Thu, May 20, 2021 at 3:16 PM Mircea Trofin <mtrofin at google.com>
>>>>>> wrote:
>>>>>>
>>>>>>> Ah, I see. There's a precedent for using custom InlineAdvisors, see
>>>>>>> for instance (in Inliner.cpp) how the ReplayInlineAdvisor is handled. I
>>>>>>> suppose you could do the same?
>>>>>>>
>>>>>>> On Thu, May 20, 2021 at 12:04 AM Neil Henning <
>>>>>>> neil.henning at unity3d.com> wrote:
>>>>>>>
>>>>>>>> So what I need is for the default LLVM inliner to be able to use my
>>>>>>>> InlineAdvisor in some fashion - without modifying tip LLVM *locally
>>>>>>>> *to do so.
>>>>>>>>
>>>>>>>> So what I think I will have to do is land one of the proposals I
>>>>>>>> stated originally (or a better idea from any of you fine folk) into LLVM *before
>>>>>>>> *the LLVM 13 cutoff, so that when we pick up the LLVM 13 release
>>>>>>>> in future we'll have the APIs available to set our own InlinerAdvisor.
>>>>>>>>
>>>>>>>> So to be clear - I'm totally ok to do a patch to LLVM to fix this,
>>>>>>>> I just can't patch LLVM myself *locally *post-release because we
>>>>>>>> are provided with a pre-built LLVM for some platforms we support.
>>>>>>>>
>>>>>>>> Hopefully that makes it a bit clearer?
>>>>>>>>
>>>>>>>> On Wed, May 19, 2021 at 4:21 PM Mircea Trofin <mtrofin at google.com>
>>>>>>>> wrote:
>>>>>>>>
>>>>>>>>>
>>>>>>>>> On Wed, May 19, 2021 at 5:27 AM Neil Henning via llvm-dev <
>>>>>>>>> llvm-dev at lists.llvm.org> wrote:
>>>>>>>>>
>>>>>>>>>> Hey list,
>>>>>>>>>>
>>>>>>>>>> I'm currently porting our HPC# Burst compiler over from the
>>>>>>>>>> legacy pass manager to the new pass manager. While nearly everything went
>>>>>>>>>> fine, I've hit one major hiccup that I can't seem to workaround - how can
>>>>>>>>>> we have a custom `InlineAdvisor` for Burst without modifying tip LLVM.
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>> I'm trying to understand this better - you mean you'd want to load
>>>>>>>>> the InlineAdvisor from a dynamic library, or something like that?
>>>>>>>>>
>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>> At present I've managed to completely bodge this locally by
>>>>>>>>>> getting access to the `OwnedAdvisor` member of `InlinerPass` through very
>>>>>>>>>> UB means (make a class of the same layout, casteroo, assign the field). Now
>>>>>>>>>> this works in that I don't have codegen regressions anymore, but obviously
>>>>>>>>>> this isn't the solution I want to ship!
>>>>>>>>>>
>>>>>>>>>> I was wondering if the list would object to us either:
>>>>>>>>>>
>>>>>>>>>>    1. Making the `OwnedAdvisor` field of `InlinerPass`
>>>>>>>>>>    protected, so I could derive from `InlinerPass` and set the advisor.
>>>>>>>>>>    2. I could make the `getAdvisor` virtual, and assign it that
>>>>>>>>>>    way.
>>>>>>>>>>    3. Probably the 'best' fix would be to make
>>>>>>>>>>    `InlineAdvisorAnalysis` somehow able to take a user-provided
>>>>>>>>>>    `InlineAdvisor` - although I'd rather not use the static option
>>>>>>>>>>    `UseInlineAdvisor` to set this. I don't really know how this solution would
>>>>>>>>>>    look if I'm honest.
>>>>>>>>>>
>>>>>>>>>> I'm trying to understand what amount of changes to tip of tree
>>>>>>>>> are OK for your scenario. Option 1 means modifying a .h; maybe option 2
>>>>>>>>> needs a recompile though (because virtual). So would option 3 (at this
>>>>>>>>> point, we can talk about purpose-building support for your scenario,
>>>>>>>>> basically - if rebuilding the compiler binaries is on the table)
>>>>>>>>>
>>>>>>>>> Thoughts from anyone? This is a blocker for us in the LLVM 13
>>>>>>>>>> timeframe when we hope to enable the new pass manager as the default.
>>>>>>>>>>
>>>>>>>>>> Cheers,
>>>>>>>>>> -Neil.
>>>>>>>>>>
>>>>>>>>>> --
>>>>>>>>>> Neil Henning
>>>>>>>>>> Senior Software Engineer Compiler
>>>>>>>>>> unity.com
>>>>>>>>>> _______________________________________________
>>>>>>>>>> LLVM Developers mailing list
>>>>>>>>>> llvm-dev at lists.llvm.org
>>>>>>>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>>>>>>>
>>>>>>>>>
>>>>>>>>
>>>>>>>> --
>>>>>>>> Neil Henning
>>>>>>>> Senior Software Engineer Compiler
>>>>>>>> unity.com
>>>>>>>>
>>>>>>>
>>>>>>
>>>>>> --
>>>>>> Neil Henning
>>>>>> Senior Software Engineer Compiler
>>>>>> unity.com
>>>>>>
>>>>>
>>>>
>>>> --
>>>> Neil Henning
>>>> Senior Software Engineer Compiler
>>>> unity.com
>>>> _______________________________________________
>>>> LLVM Developers mailing list
>>>> llvm-dev at lists.llvm.org
>>>> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>>>>
>>>
>
> --
> Neil Henning
> Senior Software Engineer Compiler
> unity.com
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210520/96ad4980/attachment.html>

More information about the llvm-dev mailing list