[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass

Carrot added a comment. 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? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass

Carrot added a comment. Finally complain comes. It slows down a hot path in tcmalloc. A simplified test case is at https://godbolt.org/z/T6sshjf7M 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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe52364532afb: [NewPM] Remove SpeculateAroundPHIs pass (authored by lebedev.ri). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION h

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass

lebedev.ri added a comment. I see. Going to land this now. Thanks for looking! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D104099/new/ https://reviews.llvm.org/D104099 ___ cfe-commits mailing list cfe-

[PATCH] D104099: [NewPM] Remove SpeculateAroundPHIs pass

aeubanks accepted this revision. aeubanks added a comment. looks good, we can always revive the pass later if somebody wants it for some reason 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

lebedev.ri updated this revision to Diff 352176. lebedev.ri retitled this revision from "[NewPM] Remove SpeculateAroundPHIs pass from pipeline" to "[NewPM] Remove SpeculateAroundPHIs pass". lebedev.ri added a comment. Herald added a subscriber: mgorny. On Tue, Jun 15, 2021 at 8:14 PM Wei Mi wrot

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 comm

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 wor

[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 th

[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/ https://reviews.llvm.org/D1040

[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 m

[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 sa

[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/ https://reviews.llvm.org/D10409

[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 t

[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 an

[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: clang/test/CodeGen/thinlto-d

[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: clang/test

[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 in

[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 (get

[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 while

[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 subs