[llvm] [Analysis] Remove global state from `PluginInline{Advisor,Order}Analysis`. (PR #114615)

Michele Scandale via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 13 10:03:18 PST 2024


michele-scandale wrote:

> > This causes issues when pipelines using plugins and pipelines not using plugins are run in the same process.
> 
> I think I'm not understanding how
> 
> ```
> llvmGetPassPluginInfo() {
>  return {
>    [](PassBuilder &PB) {
>              PB.registerPipelineStartEPCallback(
>                  [](ModulePassManager &MPM, OptimizationLevel Level) {
>                    MPM.addPass(DefaultDynamicAdvisor());
>                  });
>            }};
>  }
> ```
> 
> with this patch applied is any different from just
> 
> ```
> llvmGetPassPluginInfo() {
>  return {
>    [](PassBuilder &PB) {
>              PB.registerAnalysisRegistrationCallback(
>                  [](ModuleAnalysisManager & MAM) {
>                    PluginInlineAdvisorAnalysis PA(defaultAdvisorFactory);
>                    MAM.registerPass([&] { return PA; });
>                  });
>            }};
>  }
> ```
> 
> with the `DefaultDynamicAdvisor` pass removed. `registerAnalysisRegistrationCallback` seems like a cleaner way of registering the inline advisor unless I'm not understanding some other detail of `DefaultDynamicAdvisor`

The problem is on the `InlineAdvisor` side when the `PluginInlineAdvisorAnalysis ` is being queried:
```
bool InlineAdvisorAnalysis::Result::tryCreate(
    InlineParams Params, InliningAdvisorMode Mode,
    const ReplayInlinerSettings &ReplaySettings, InlineContext IC) {
  auto &FAM = MAM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
  if (PluginInlineAdvisorAnalysis::HasBeenRegistered) { // <---- this is a shared global variable!
    auto &DA = MAM.getResult<PluginInlineAdvisorAnalysis>(M);
    Advisor.reset(DA.Factory(M, FAM, Params, IC));
    return !!Advisor;
  }
```

the `HasBeenRegistered` global variable  is being set to `true` in the constructor of `PluginInlineAdvisorAnalysis` -- which currently happen when  the `DefaultDynamicAdvisor::run` is executed, and with your change it will happen when the analysis are registered.
In both cases once someone somewhere does that the `HasBeenRegistered` applies globally, and that will affect whatever other pipelines where plugins are not being loaded.

https://github.com/llvm/llvm-project/pull/114615


More information about the llvm-commits mailing list