[NewPM] Remove SpeculateAroundPHIs pass

[lebedev.ri]

Summary

Addition of this pass has been botchered.
There is no particular reason why it had to be sold as an inseparable part of new-pm transition.
It was added when old-pm was still the default, and very *very* few users were actually tracking new-pm,
so it's effects weren't measured. Which means, some of the turnoil of the new-pm transition
are actually likely regressions due to this pass.

Likewise, there has been a number of post-commit feedback (post new-pm switch), namely

• https://reviews.llvm.org/D37467#2787157 (regresses HW-loops)
• https://reviews.llvm.org/D37467#2787259 (should not be in middle-end, should run after LSR, not before)
• https://reviews.llvm.org/D95789 (an attempt to fix bad loop backedge metadata)

and in the half year past, the pass authors (google) still haven't found time to respond to any of that.

Hereby it is proposed to backout the pass from the pipeline, until someone who cares about it
can address the issues reported, and properly start the process
of adding a new pass into the pipeline, with proper performance evaluation.

[nikic]

Looks like phase ordering tests need an update.

llvm/lib/Passes/PassBuilder.cpp

1449

As it has been in-tree for a while, I think it would be good to add a cl::opt option for people who have come to rely on it being present in some way.

[lebedev.ri]

llvm/lib/Passes/PassBuilder.cpp

1449

I'm not convinced that the pass can be returned into it's current position in pipeline,
that is why the diff is what it is now.

[thopre]

+1 for this change but being a downstream target only I prefer to let someone else approve this.

[bjope]

Kind of the same take on this as @thopre .

I wrote https://bugs.llvm.org/show_bug.cgi?id=48821 , and proposed https://reviews.llvm.org/D95789 , when I noticed that this pass caused troubles for two reasons:

1. to avoid that others would stumble upon the same problem (getting unexpected regressions due to switching pass manager)
2. not that important really, but getting rid of downstream diffs is always nice (at least when the problem is likely to cause problems for other targets as well)

For the downstream target I've hacked around this a long time ago.

I do think it was a bit sneaky to get this pass by default in the pipeline as part of the PM switch. It might have caused lots of trouble for the new-PM switch in the past, and still for some downstream contributors.
So in that sense I'm all in favor. Although, a LGTM from me is a bit weak as I'm already using a workaround to skip this pass downstream.

[bjope]

llvm/lib/Passes/PassBuilder.cpp

1449

Yes, there is a contradiction between the code comment and the position in the pipeline. I did question that here: https://bugs.llvm.org/show_bug.cgi?id=48821#c2

[lebedev.ri]

Update a few more tests.

[lebedev.ri]

... and upload the right patch this time.

[aeubanks]

Some backends don't run SimplifyCFG, e.g. X86. I believe the pass was originally created specifically for X86 (the header has some X86 examples) and may or may not extend to other targets (I'm not very familiar with the pass itself).

I'm not opposed to landing this and seeing who complains, but if somebody does, we can make this pass X86-specific by adding it to X86TargetMachine::registerPassBuilderCallbacks() (which doesn't exist yet).

[lebedev.ri]

In D104099#2813713, @aeubanks wrote:

Some backends don't run SimplifyCFG, e.g. X86. I believe the pass was originally created specifically for X86 (the header has some X86 examples) and may or may not extend to other targets (I'm not very familiar with the pass itself).

I'm not opposed to landing this and seeing who complains, but if somebody does, we can make this pass X86-specific by adding it to X86TargetMachine::registerPassBuilderCallbacks() (which doesn't exist yet).

I am complaining about X86 side of things :)
It needs to be *at least* a late IR pass in codegen pipeline.

[aeubanks]

Which pass that comes after SpeculateAroundPHIs in the X86 pipeline (either in the optimization or codegen) would undo its effects?

[lebedev.ri]

In D104099#2813743, @aeubanks wrote:

Which pass that comes after SpeculateAroundPHIs in the X86 pipeline (either in the optimization or codegen) would undo its effects?

As i have wrote in some other review, the pass i saw as causing problems is LSR.

[aeubanks]

Ah, sorry I missed where you mentioned LSR.

If this pass is causing regressions in multiple places, even on X86, then I think it does make sense to remove it.
I've added some people that may care about this pass more than me. Maybe wait a bit for their feedback, if they have any.

[davidxl]

Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads.

[wenlei]

In D104099#2814167, @davidxl wrote:

Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads.

Measured perf using FB internal workload w/ and w/o this pass, result is neutral.

[lebedev.ri]

In D104099#2815531, @wenlei wrote:

In D104099#2814167, @davidxl wrote:

Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads.

Measured perf using FB internal workload w/ and w/o this pass, result is neutral.

Thank you for checking!

So far, it seems the reaction to this proposal has been overwhelmingly positive.
Does anyone else wish to chime in? Should i land this? @asbirlea @MaskRay ?

[lebedev.ri]

On Tue, Jun 15, 2021 at 8:14 PM Wei Mi <[e-mail]> wrote:

On Mon, Jun 14, 2021 at 4:52 PM Wei Mi <[e-mail]> wrote:

On Mon, Jun 14, 2021 at 4:04 PM Xinliang David Li <[e-mail]> wrote:

On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator <[e-mail]> wrote:

lebedev.ri added a subscriber: MaskRay.
lebedev.ri added a comment.

In D104099#2815531 https://reviews.llvm.org/D104099#2815531, @wenlei wrote:

In D104099#2814167 https://reviews.llvm.org/D104099#2814167, @davidxl wrote:

Adding Wei to help measure performance impact on our internal workloads.  Also add Wenlei to help measure impact with FB's workloads.

Measured perf using FB internal workload w/ and w/o this pass, result is neutral.

Thank you for checking!

So far, it seems the reaction to this proposal has been overwhelmingly positive.
Does anyone else wish to chime in? Should i land this? @asbirlea @MaskRay ?

Wei is doing more measurement @Google. Please wait for the response.

David

Start doing the test. Will report back.

Wei.

No performance change found in google internal benchmarks.

 Wei.

Thanks for checking.
So, so far no one can point out why this pass is beneficial? :]

How about this then?

 

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104099/new/

https://reviews.llvm.org/D104099

[aeubanks]

looks good, we can always revive the pass later if somebody wants it for some reason

[lebedev.ri]

I see. Going to land this now. Thanks for looking!

[Carrot]

Finally complain comes.

It slows down a hot path in tcmalloc. A simplified test case is at https://godbolt.org/z/T6sshjf7M

[Carrot]

I'm trying to understand the problem in this pass.
It looks both of @thopre's and @lebedev.ri's problems are loop related. Is there any other regression not loop related?

clang/test/CodeGen/thinlto-distributed-newpm.ll

 ; CHECK-O: Running pass: LoopSimplifyPass on main
 ; CHECK-O: Running pass: LCSSAPass on main
 ; CHECK-O: Running pass: LICMPass on Loop at depth 1 containing: %b
 ; CHECK-O: Running pass: AlignmentFromAssumptionsPass on main
 ; CHECK-O: Running pass: LoopSinkPass on main
 ; CHECK-O: Running pass: InstSimplifyPass on main
 ; CHECK-O: Running pass: DivRemPairsPass on main
 ; CHECK-O: Running pass: SimplifyCFGPass on main
-; CHECK-O: Running pass: SpeculateAroundPHIsPass on main
 ; CHECK-O: Running pass: CGProfilePass
 ; CHECK-O: Running pass: GlobalDCEPass
 ; CHECK-O: Running pass: ConstantMergePass
 
 target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
 target triple = "x86_64-grtev4-linux-gnu"
 
 define i32 @main() {
   br label %b
 b:
   br label %b
   ret i32 0
 }

llvm/lib/Passes/PassBuilder.cpp

PassBuilder::buildModuleOptimizationPipeline(OptimizationLevel Level,
   // passes to avoid re-sinking, but before SimplifyCFG because it can allow
   // flattening of blocks.
   OptimizePM.addPass(DivRemPairsPass());
 
   // LoopSink (and other loop passes since the last simplifyCFG) might have
   // resulted in single-entry-single-exit or empty blocks. Clean up the CFG.
   OptimizePM.addPass(SimplifyCFGPass());
 
-  // Optimize PHIs by speculating around them when profitable. Note that this
-  // pass needs to be run after any PRE or similar pass as it is essentially
-  // inserting redundancies into the program. This even includes SimplifyCFG.
-  OptimizePM.addPass(SpeculateAroundPHIsPass());
-
   if (PTO.Coroutines)
     OptimizePM.addPass(CoroCleanupPass());
 
   // Add the core optimizing pipeline.
   MPM.addPass(createModuleToFunctionPassAdaptor(std::move(OptimizePM)));
 
   for (auto &C : OptimizerLastEPCallbacks)
     C(MPM, Level);

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

 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass
 ; CHECK-O-NEXT: Running pass: LCSSAPass
 ; CHECK-O-NEXT: Running pass: LICMPass
 ; CHECK-O-NEXT: Running pass: AlignmentFromAssumptionsPass
 ; CHECK-O-NEXT: Running pass: LoopSinkPass
 ; CHECK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-EP-OPTIMIZER-LAST: Running pass: NoOpFunctionPass
 ; CHECK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: ConstantMergePass
 ; CHECK-DEFAULT-NEXT: Running pass: RelLookupTableConverterPass
 ; CHECK-LTO-NOT: Running pass: RelLookupTableConverterPass
 ; CHECK-DEFAULT-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-LTO-NOT: Running analysis: TargetIRAnalysis

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

 ; CHECK-POSTLINK-O-NEXT: Running pass: LoopSimplifyPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: LCSSAPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: LICMPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: AlignmentFromAssumptionsPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: LoopSinkPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-POSTLINK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-POSTLINK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-POSTLINK-O-NEXT: Running pass: ConstantMergePass
 ; CHECK-POSTLINK-O-NEXT: Running pass: RelLookupTableConverterPass
 ; CHECK-POSTLINK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT:          Running pass: AnnotationRemarksPass on foo
 ; CHECK-PRELINK-O-NEXT: Running pass: CanonicalizeAliasesPass
 ; CHECK-PRELINK-O-NEXT: Running pass: NameAnonGlobalPass

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

 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass
 ; CHECK-O-NEXT: Running pass: LCSSAPass
 ; CHECK-O-NEXT: Running pass: LICMPass
 ; CHECK-O-NEXT: Running pass: AlignmentFromAssumptionsPass
 ; CHECK-O-NEXT: Running pass: LoopSinkPass
 ; CHECK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: ConstantMergePass
 ; CHECK-O-NEXT: Running pass: RelLookupTableConverterPass
 ; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
 ; CHECK-O-NEXT: Running pass: PrintModulePass
 

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

 ; CHECK-O-NEXT: Running pass: LoopSimplifyPass
 ; CHECK-O-NEXT: Running pass: LCSSAPass
 ; CHECK-O-NEXT: Running pass: LICMPass
 ; CHECK-O-NEXT: Running pass: AlignmentFromAssumptionsPass
 ; CHECK-O-NEXT: Running pass: LoopSinkPass
 ; CHECK-O-NEXT: Running pass: InstSimplifyPass
 ; CHECK-O-NEXT: Running pass: DivRemPairsPass
 ; CHECK-O-NEXT: Running pass: SimplifyCFGPass
-; CHECK-O-NEXT: Running pass: SpeculateAroundPHIsPass
 ; CHECK-O-NEXT: Running pass: CGProfilePass
 ; CHECK-O-NEXT: Running pass: GlobalDCEPass
 ; CHECK-O-NEXT: Running pass: ConstantMergePass
 ; CHECK-O-NEXT: Running pass: RelLookupTableConverterPass
 ; CHECK-O-NEXT: Running analysis: TargetIRAnalysis
 ; CHECK-O-NEXT: Running pass: AnnotationRemarksPass on foo
 ; CHECK-O-NEXT: Running pass: PrintModulePass