llvm dev - May 2021 - How to use a custom InlineAdvisor with the new pass manager

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
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210520/2ce3c6f6/attachment-0001.html>

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20210520/b3bd651f/attachment.html>