Neil Henning via llvm-dev
2021-May-20 15:14 UTC
[llvm-dev] How to use a custom InlineAdvisor with the new pass manager
I can't edit InlineAdvisorAnalysis::Result::tryCreate to add my own InlineAdvisor there is the issue. 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. 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/8f600907/attachment.html>
Mircea Trofin via llvm-dev
2021-May-20 15:45 UTC
[llvm-dev] 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>