Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

On Mon, Jun 14, 2021 at 4:52 PM Wei Mi wrote: > > > On Mon, Jun 14, 2021 at 4:04 PM Xinliang David Li > wrote: > >> >> >> On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator < >> revi...@reviews.llvm.org> wrote: >> >>> lebedev.ri added a subscriber: MaskRay. >>> lebedev.ri added a

Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

On Mon, Jun 14, 2021 at 4:04 PM Xinliang David Li wrote: > > > On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator < > revi...@reviews.llvm.org> wrote: > >> lebedev.ri added a subscriber: MaskRay. >> lebedev.ri added a comment. >> >> In D104099#2815531

Re: [PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

On Mon, Jun 14, 2021 at 3:59 PM Roman Lebedev via Phabricator < revi...@reviews.llvm.org> wrote: > lebedev.ri added a subscriber: MaskRay. > lebedev.ri added a comment. > > In D104099#2815531 , @wenlei > wrote: > > > In D104099#2814167

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri added a subscriber: MaskRay. lebedev.ri added a comment. In D104099#2815531 , @wenlei wrote: > In D104099#2814167 , @davidxl wrote: > >> Adding Wei to help measure performance impact on our internal

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

wenlei added a comment. 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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

davidxl added a comment. Adding Wei to help measure performance impact on our internal workloads. Also add Wenlei to help measure impact with FB's workloads. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

aeubanks accepted this revision. aeubanks added a comment. This revision is now accepted and ready to land. 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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri added a comment. 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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

aeubanks added a comment. Which pass that comes after SpeculateAroundPHIs in the X86 pipeline (either in the optimization or codegen) would undo its effects? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri added a comment. 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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

aeubanks added a comment. 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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri updated this revision to Diff 351445. lebedev.ri added a comment. ... and upload the right patch this time. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/ https://reviews.llvm.org/D104099 Files:

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri updated this revision to Diff 351444. lebedev.ri added a comment. Herald added a subscriber: zzheng. Update a few more tests. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/ https://reviews.llvm.org/D104099 Files:

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

bjope added inline comments. Comment at: llvm/lib/Passes/PassBuilder.cpp:1449 - // inserting redundancies into the program. This even includes SimplifyCFG. - OptimizePM.addPass(SpeculateAroundPHIsPass()); - lebedev.ri wrote: > nikic wrote: > > As it has been

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

bjope added a comment. 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

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

thopre added a comment. +1 for this change but being a downstream target only I prefer to let someone else approve this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/ https://reviews.llvm.org/D104099

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri added inline comments. Comment at: llvm/lib/Passes/PassBuilder.cpp:1449 - // inserting redundancies into the program. This even includes SimplifyCFG. - OptimizePM.addPass(SpeculateAroundPHIsPass()); - nikic wrote: > As it has been in-tree for a

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

nikic added a comment. Looks like phase ordering tests need an update. Comment at: llvm/lib/Passes/PassBuilder.cpp:1449 - // inserting redundancies into the program. This even includes SimplifyCFG. - OptimizePM.addPass(SpeculateAroundPHIsPass()); - As it has

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass from pipeline

lebedev.ri created this revision. lebedev.ri added reviewers: thopre, bjope, nikic, chandlerc, aeubanks. lebedev.ri added a project: LLVM. Herald added subscribers: ormris, wenlei, steven_wu, hiraditya. lebedev.ri requested review of this revision. Herald added a project: clang. Herald added a