Mircea Trofin via llvm-dev
2021-May-20 18:32 UTC
[llvm-dev] How to use a custom InlineAdvisor with the new pass manager
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 >> >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20210520/0ef17609/attachment.html>
Neil Henning via llvm-dev
2021-May-20 18:48 UTC
[llvm-dev] How to use a custom InlineAdvisor with the new pass manager
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/4a61d7c7/attachment.html>