How to use a custom InlineAdvisor with the new pass manager

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.

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.
    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.

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)

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?

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?

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?

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!

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
{
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.getResultllvm::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.

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!).

I think I start to understand, is this correct: you set up your own optimization pipeline. You don’t use ModuleInlinerWrapperPass, instead you set up the InlinerPass and (presumably) you’d RequireAnalysisPass with the InlineAdvisorAnalysis.

What about having the InlineAdvisorAnalysis ctor take a std::function<std::unique_ptr()> parameter? This way, the Advisor is created when needed, and we can fold into that design the current tryCreate. I think we may be able to even avoid the “owned advisor” thing in Inliner.cpp

If this would address your scenario, I’d be happy to do the refactoring!

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”) - 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

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 :slight_smile: thanks!

Ok cool! I think that’d work for us. I’ll put together a PR and add you as reviewers if that’s ok!

Yup, happy to help with the review!