[llvm] r363287 - [clang][NewPM] Fix broken -O0 test from missing assumptions

[Michael Kruse via llvm-commits]

I committed the registration of the FunctionAnalysisManagerModuleProxy
in the ModuleAnalysisManager in r363572 as a first fix. Thank you the
suggestion.

In the longer term, it doesn't seem to scale well if users of the new
pass manager need to manually register (all/some) passes that are
transitively required. Probably PassBuilder::crossRegisterProxies
should be used, but it requires many more AnalysisManagers to be
set-up.

Solution  2) does not work because the AlwaysInliner should run after
the ScopInliner (to carry-out the inlining suggested by the
ScopInliner).

Michael



Am Sa., 15. Juni 2019 um 02:06 Uhr schrieb Chandler Carruth
<chandlerc at gmail.com>:
>
> On Fri, Jun 14, 2019 at 10:56 PM Leonard Chan <leonardchan at google.com> wrote:
>>
>> Thanks for bringing this up. I'm not sure how polly works, but I think I found out the root of the problem.
>>
>> For new pass manager runs, the FunctionAnalysisManagerModuleProxy is registered in PassBuilder::crossRegisterProxies(). `opt` calls this in `llvm::runPassPipeline()` but only if the new PM driver is used (which can be triggered by passing `-passes='...'`). The command in question though does not seem to be using the new PM, so the problem is that this new PM pass is being used in a legacy PM run.
>>
>> After some digging, it seems that the new PM AlwaysInlinerPass (and some other new PM passes) are created and run in `polly/trunk/lib/Transform/ScopInliner.cpp` as a part of ScopInliner::runOnSCC(), which should be a legacy pass.
>>
>> I'd double check this with Chandler or Philip, but I imagine the correct thing to do is to instead change some of the body of ScopInliner::runOnSCC() to 1) use the legacy PM classes (like AlwaysInlinerLegacyPass), and 2) change ScopInliner to have the AlwaysInliner as a pass dependency instead of invoking it directly in the run.
>
>
> Or 3) register and set up the analysis manager inside of the ScopInliner.
>
> Nothing goes wrong by nesting new PM inside of something else. You just have to build and set up the analysis manager correctly to hand to the underlying pass.
>
> FWIW, I agree w/ your analysis of the problem and the available solutions. I don't have a strong opinion. It'd be good to land one of these ore temporarily revert until we figure out what to do.... #3 is honestly probably the minimal change -- it should allow things to work exactly as they did before.
>
>>
>>
>> - Leonard
>>
>> On Fri, Jun 14, 2019 at 9:20 PM Michael Kruse <llvm-commits at meinersbur.de> wrote:
>>>
>>> This made two regression tests in Polly crash:
>>>
>>> Debug\bin\opt.exe  -polly-process-unprofitable  -polly-remarks-minimal
>>>  -polly-use-llvm-names
>>> -polly-import-jscop-dir=C:\Users\meinersbur\src\llvm\tools\polly\test\ScopInliner
>>>  -polly-codegen-verify  -polly-detect-full-functions
>>> -polly-scop-inliner  -polly-scops -analyze
>>> -polly-invariant-load-hoisting
>>> Assertion failed: AnalysisPasses.count(PassT::ID()) && "This analysis
>>> pass was not registered prior to being queried", file
>>> C:\Users\meinersbur\src\llvm\include\llvm/IR/PassManager.h, line 778
>>>
>>> with PassT being FunctionAnalysisManagerModuleProxy. Where is this
>>> pass supposed to be registered?
>>>
>>> Michael
>>>
>>>
>>> Am Do., 13. Juni 2019 um 13:15 Uhr schrieb Leonard Chan via
>>> llvm-commits <llvm-commits at lists.llvm.org>:
>>> >
>>> > Author: leonardchan
>>> > Date: Thu Jun 13 11:18:40 2019
>>> > New Revision: 363287
>>> >
>>> > URL: http://llvm.org/viewvc/llvm-project?rev=363287&view=rev
>>> > Log:
>>> > [clang][NewPM] Fix broken -O0 test from missing assumptions
>>> >
>>> > Add an AssumptionCache callback to the InlineFuntionInfo used for the
>>> > AlwaysInlinerPass to match codegen of the AlwaysInlinerLegacyPass to generate
>>> > llvm.assume. This fixes CodeGen/builtin-movdir.c when new PM is enabled by
>>> > default.
>>> >
>>> > Differential Revision: https://reviews.llvm.org/D63170
>>> >
>>> > Modified:
>>> >     llvm/trunk/lib/Transforms/IPO/AlwaysInliner.cpp
>>> >
>>> > Modified: llvm/trunk/lib/Transforms/IPO/AlwaysInliner.cpp
>>> > URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/IPO/AlwaysInliner.cpp?rev=363287&r1=363286&r2=363287&view=diff
>>> > ==============================================================================
>>> > --- llvm/trunk/lib/Transforms/IPO/AlwaysInliner.cpp (original)
>>> > +++ llvm/trunk/lib/Transforms/IPO/AlwaysInliner.cpp Thu Jun 13 11:18:40 2019
>>> > @@ -31,8 +31,17 @@ using namespace llvm;
>>> >
>>> >  #define DEBUG_TYPE "inline"
>>> >
>>> > -PreservedAnalyses AlwaysInlinerPass::run(Module &M, ModuleAnalysisManager &) {
>>> > -  InlineFunctionInfo IFI;
>>> > +PreservedAnalyses AlwaysInlinerPass::run(Module &M,
>>> > +                                         ModuleAnalysisManager &MAM) {
>>> > +  // Add inline assumptions during code generation.
>>> > +  FunctionAnalysisManager &FAM =
>>> > +      MAM.getResult<FunctionAnalysisManagerModuleProxy>(M).getManager();
>>> > +  std::function<AssumptionCache &(Function &)> GetAssumptionCache =
>>> > +      [&](Function &F) -> AssumptionCache & {
>>> > +    return FAM.getResult<AssumptionAnalysis>(F);
>>> > +  };
>>> > +  InlineFunctionInfo IFI(/*cg=*/nullptr, &GetAssumptionCache);
>>> > +
>>> >    SmallSetVector<CallSite, 16> Calls;
>>> >    bool Changed = false;
>>> >    SmallVector<Function *, 16> InlinedFunctions;
>>> >
>>> >
>>> > _______________________________________________
>>> > llvm-commits mailing list
>>> > llvm-commits at lists.llvm.org
>>> > https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits