[Attributor] Add the Attributor to the new PM pipeline

[jdoerfert]

Summary

For now, this matches the old pipeline where the Attributor is run early
as module pass (but disabled by default through an internal flag).

We are working on an CG-SCC pass version that we can run later as well.

[lebedev.ri]

Not really familiar with NewPM..

[leonardchan]

Adding @chandlerc for review. I'm not very comfortable reviewing changes to the backend pipeline.

Could you also add an LLVM test showing how this pass should affect the IR?

[jdoerfert]

In D69596#1728978, @leonardchan wrote:

Adding @chandlerc for review. I'm not very comfortable reviewing changes to the backend pipeline.

Could you also add an LLVM test showing how this pass should affect the IR?

It will not affect the IR without being turned on.

[jdoerfert]

include the test changes this time

[fedor.sergeev]

Is there a particular reason for choosing this spot in pipeline?
I see that in legacy pipeline Attributor follows IPSCCP/CVP, and here it goes right before.

[fedor.sergeev]

So, this enables Attributor pass, but in fact has all its functionality disabled unless -attributor-disable is set to false.
Is there any testing that does enable attributor?
If yes, then I would like to see this testing being run with new pass manager as well.

[fedor.sergeev]

Is there any testing that does enable attributor?

Okey, I did find a fair amount of tests that run individual -attributor *or* -passes=attributor (with -attributor-disable=false).
Are there any near plans to enable attributor (setting -attributor-disable=false by default)?

I dont really see a sense in having AttributorPass in pipeline but all the sematics of it completely disabled, so running Attributor does not really runs the attributor.

If -attributor-disabled=true is the state that you expect it to exist for a prolonged period of time I would better see -attributor-disable option
controlling conditional inclusion of this pass into the pipeline.

This way when somebody sees "Running pass: AttributorPass" there will be no question on whether Attributor ran or not.

[fedor.sergeev]

setting request-changes state to make it obvious that there are some questions here I would like to see answered.

[jdoerfert]

In D69596#1783188, @fedor.sergeev wrote:

Is there any testing that does enable attributor?

Okey, I did find a fair amount of tests that run individual -attributor *or* -passes=attributor (with -attributor-disable=false).
Are there any near plans to enable attributor (setting -attributor-disable=false by default)?

Yes. I did fix the remaining LLVM test suite bugs the other day again. I was hoping to land the SCC version (D70767) before I ask people to set the disabled=false flag in their local test setup and report any breakages/problems.

I dont really see a sense in having AttributorPass in pipeline but all the sematics of it completely disabled, so running Attributor does not really runs the attributor.

It allows folks to test it locally by setting the flag. Without it being in the pipeline that is, as far as I know, really hard to do.

If -attributor-disabled=true is the state that you expect it to exist for a prolonged period of time I would better see -attributor-disable option
controlling conditional inclusion of this pass into the pipeline.

We can do that but (1) I hope disabled will default to false in the next release and (2) I want to make sure we do not invalidate any pass result.

This way when somebody sees "Running pass: AttributorPass" there will be no question on whether Attributor ran or not.

Fair point. Once we enabled it by default I would be happy with not putting it in the pipeline if the flag is set. I can do that now if you think it helps.

In D69596#1841810, @fedor.sergeev wrote:

setting request-changes state to make it obvious that there are some questions here I would like to see answered.

Sorry, I wrote the SCC pass (D70767) and merged this into it without closing this revision.

llvm/lib/Passes/PassBuilder.cpp

if (LoadSampleProfile) {
     if (Phase != ThinLTOPhase::PreLink)
       // We perform early indirect call promotion here, before globalopt.
       // This is important for the ThinLTO backend phase because otherwise
       // imported available_externally functions look unreferenced and are
       // removed.
       MPM.addPass(PGOIndirectCallPromotion(Phase == ThinLTOPhase::PostLink,
                                            true /* SamplePGO */));
   }
+  MPM.addPass(AttributorPass());
 
   // Interprocedural constant propagation now that basic cleanup has occurred
   // and prior to optimizing globals.
   // FIXME: This position in the pipeline hasn't been carefully considered in
   // years, it should be re-analyzed.
   MPM.addPass(IPSCCPPass());
 
   // Attach metadata to indirect call sites indicating the set of functions

llvm/test/Other/new-pm-defaults.ll

 ; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
 ; CHECK-O-NEXT: Running pass: SROA
 ; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
 ; CHECK-O3-NEXT: Running pass: CallSiteSplittingPass
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
+; CHECK-O-NEXT: Running pass: AttributorPass
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
 ; CHECK-O-NEXT: Running pass: CalledValuePropagationPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PromotePass>
 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: InstCombinePass

llvm/test/Other/new-pm-thinlto-defaults.ll

 ; CHECK-O-NEXT: Running analysis: AssumptionAnalysis
 ; CHECK-O-NEXT: Running pass: SROA
 ; CHECK-O-NEXT: Running analysis: DominatorTreeAnalysis
 ; CHECK-O-NEXT: Running pass: EarlyCSEPass
 ; CHECK-O-NEXT: Running analysis: TargetLibraryAnalysis
 ; CHECK-O-NEXT: Running pass: LowerExpectIntrinsicPass
 ; CHECK-O3-NEXT: Running pass: CallSiteSplittingPass
 ; CHECK-O-NEXT: Finished llvm::Function pass manager run.
+; CHECK-O-NEXT: Running pass: AttributorPass
 ; CHECK-O-NEXT: Running pass: IPSCCPPass
 ; CHECK-O-NEXT: Running pass: CalledValuePropagationPass
 ; CHECK-O-NEXT: Running pass: GlobalOptPass
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PromotePass>
 ; CHECK-O-NEXT: Running pass: DeadArgumentEliminationPass
 ; CHECK-O-NEXT: Running pass: ModuleToFunctionPassAdaptor<{{.*}}PassManager{{.*}}>
 ; CHECK-O-NEXT: Starting llvm::Function pass manager run.
 ; CHECK-O-NEXT: Running pass: InstCombinePass