Page MenuHomePhabricator

[NewPM] Remove SpeculateAroundPHIs pass
ClosedPublic

Authored by lebedev.ri on Fri, Jun 11, 2:07 AM.

Details

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

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.

Diff Detail

Event Timeline

lebedev.ri created this revision.Fri, Jun 11, 2:07 AM
lebedev.ri requested review of this revision.Fri, Jun 11, 2:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Jun 11, 2:07 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nikic added a comment.Fri, Jun 11, 3:33 AM

Looks like phase ordering tests need an update.

llvm/lib/Passes/PassBuilder.cpp
1454

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 added inline comments.Fri, Jun 11, 3:42 AM
llvm/lib/Passes/PassBuilder.cpp
1454

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.

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

bjope added a comment.Fri, Jun 11, 6:24 AM

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 added inline comments.Fri, Jun 11, 6:27 AM
llvm/lib/Passes/PassBuilder.cpp
1454

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

Update a few more tests.

... and upload the right patch this time.

lebedev.ri edited the summary of this revision. (Show Details)Fri, Jun 11, 7:39 AM
lebedev.ri edited the summary of this revision. (Show Details)Fri, Jun 11, 10:00 AM

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

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.

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

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 accepted this revision.Fri, Jun 11, 11:10 AM

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.

This revision is now accepted and ready to land.Fri, Jun 11, 11:10 AM
davidxl added a subscriber: wmi.

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

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.

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 retitled this revision from [NewPM] Remove SpeculateAroundPHIs pass from pipeline to [NewPM] Remove SpeculateAroundPHIs pass.

On Tue, Jun 15, 2021 at 8:14 PM Wei Mi <wmi@google.com> wrote:

On Mon, Jun 14, 2021 at 4:52 PM Wei Mi <wmi@google.com> wrote:

On Mon, Jun 14, 2021 at 4:04 PM Xinliang David Li <davidxl@google.com> wrote:

On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator <reviews@reviews.llvm.org> 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 accepted this revision.Tue, Jun 15, 10:31 AM

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

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

This revision was landed with ongoing or failed builds.Tue, Jun 15, 10:36 AM
This revision was automatically updated to reflect the committed changes.