[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added a comment. In D83013#2143470 , @hans wrote: > Still lgtm. For what it's worth, I think you could have just re-committed > with the fixes rather than uploading for review again. This may be a difference of habits but I usually upload the la

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu added a comment. In D83013#2143470 , @hans wrote: > Still lgtm. For what it's worth, I think you could have just re-committed > with the fixes rather than uploading for review again. Gotcha, thanks. Repository: rG LLVM Github Monorepo CHAN

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG1fbb719470c6: [LPM] Port CGProfilePass from NPM to LPM (authored by zequanwu). Repository: rG LLVM Github Monorepo CHA

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans accepted this revision. hans added a comment. Still lgtm. For what it's worth, I think you could have just re-committed with the fixes rather than uploading for review again. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://re

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu marked 5 inline comments as done. zequanwu added inline comments. Comment at: llvm/lib/Transforms/Instrumentation/CGProfile.cpp:64 // Ignore error here. Indirect calls are ignored if this fails. - (void)(bool)Symtab.create(M); + (void)(bool) Symtab.create(M); fo

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 276834. zequanwu added a comment. Add comments and fix test failure in http://45.33.8.238/linux/22500/step_7.txt. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 Files

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

lebedev.ri added a comment. This seems to have broken the build http://45.33.8.238/linux/22500/step_7.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 ___ cf

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

echristo added a comment. Some inline nits. I see you've already committed and that's fine - I still don't think we should do it, but we can delete it again soon :) Comment at: clang/lib/CodeGen/BackendUtil.cpp:623 PMBuilder.LoopVectorize = CodeGenOpts.VectorizeLoop; + PMB

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGc92a8c0a0f68: [LPM] Port CGProfilePass from NPM to LPM (authored by zequanwu). Repository: rG LLVM Github Monorepo CHA

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 276813. zequanwu added a comment. rebase. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 Files: clang/include/clang/Basic/CodeGenOptions.def clang/lib/CodeGen/Bac

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 276808. zequanwu added a comment. Update test case. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 Files: clang/include/clang/Basic/CodeGenOptions.def clang/lib/C

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans accepted this revision. hans added a comment. In D83013#2142271 , @nikic wrote: > New compile-time numbers: > https://llvm-compile-time-tracker.com/compare.php?from=0b39d2d75275b80994dac06b7ad05031cbd09393&to=fd070b79e063fff2fad3cd4a467f64dfca83eb90&

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

nikic accepted this revision. nikic added a comment. LG from my side. New compile-time numbers: https://llvm-compile-time-tracker.com/compare.php?from=0b39d2d75275b80994dac06b7ad05031cbd09393&to=fd070b79e063fff2fad3cd4a467f64dfca83eb90&stat=instructions It's nearly neutral now. =

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added a comment. In D83013#2139882 , @zequanwu wrote: > > The alternative of using LazyBlockFrequencyInfoPass and checking > > PSI->hasProfileSummary() first would also work I guess. If you think that's > > cleaner, maybe that's the better way to

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu marked an inline comment as done. zequanwu added a comment. > The alternative of using LazyBlockFrequencyInfoPass and checking > PSI->hasProfileSummary() first would also work I guess. If you think that's > cleaner, maybe that's the better way to go. Since `PSI->hasProfileSummary()` is

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 276526. zequanwu added a comment. - Remove "enable-call-graph-profile" option and enable CGProfilePass by default, unless `-no-integrated-as` is given in clang. - Use `LazyBlockFrequencyInfoPass` instead of `BlockFrequencyInfoWrapperPass` and check `F.getEn

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added a comment. In D83013#2137607 , @MaskRay wrote: > `Opts.getProfileUse() != CodeGenOptions::ProfileNone ` in > > Opts.CallGraphProfile = Opts.getProfileUse() != CodeGenOptions::ProfileNone > && > !Opts.DisableIntegra

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added a comment. I still haven't seen a strong argument keeping a command line option `-enable-npm-call-graph-profile`. Asked in D62627 . `Opts.getProfileUse() != CodeGenOptions::ProfileNone ` in Opts.CallGraphProfile = Opts.getProfileUse() != CodeGen

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 276208. zequanwu marked an inline comment as done. zequanwu added a comment. Herald added subscribers: dexonsmith, steven_wu. - Disable `enable-call-graph-profile` by default in opt. - Disable `CGProfilePass` by default in clang unless `-no-integrated-as` is

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu added inline comments. Comment at: llvm/test/Other/opt-O2-pipeline.ll:289 +; CHECK-NEXT: Branch Probability Analysis +; CHECK-NEXT: Block Frequency Analysis ; CHECK-NEXT: FunctionPass Manager MaskRay wrote: > zequanwu wrote: > > zequ

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added inline comments. Comment at: llvm/test/Other/opt-O2-pipeline.ll:289 +; CHECK-NEXT: Branch Probability Analysis +; CHECK-NEXT: Block Frequency Analysis ; CHECK-NEXT: FunctionPass Manager zequanwu wrote: > zequanwu wrote: > > niki

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu marked an inline comment as done. zequanwu added inline comments. Comment at: llvm/test/Other/opt-O2-pipeline.ll:289 +; CHECK-NEXT: Branch Probability Analysis +; CHECK-NEXT: Block Frequency Analysis ; CHECK-NEXT: FunctionPass Manager ---

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu marked an inline comment as done. zequanwu added inline comments. Comment at: llvm/test/Other/opt-O2-pipeline.ll:289 +; CHECK-NEXT: Branch Probability Analysis +; CHECK-NEXT: Block Frequency Analysis ; CHECK-NEXT: FunctionPass Manager ---

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

nikic added inline comments. Comment at: llvm/test/Other/opt-O2-pipeline.ll:289 +; CHECK-NEXT: Branch Probability Analysis +; CHECK-NEXT: Block Frequency Analysis ; CHECK-NEXT: FunctionPass Manager hans wrote: > nikic wrote: > > Is it possibl

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added inline comments. Comment at: llvm/lib/Transforms/IPO/PassManagerBuilder.cpp:170 PassManagerBuilder::PassManagerBuilder() { OptLevel = 2; Oh, just noticed: I think CallGraphProfile should be initialized along with the other flags here. ==

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

nikic requested changes to this revision. nikic added inline comments. This revision now requires changes to proceed. Comment at: llvm/test/Other/opt-O2-pipeline.ll:289 +; CHECK-NEXT: Branch Probability Analysis +; CHECK-NEXT: Block Frequency Analysis ; CHECK-NEX

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans accepted this revision. hans added a comment. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added a comment. > I don't want to block this patch, but I do agree with Eric's point. We > *really* want to focus more on the switch then invest into more LPM infra. > Short term resolutions to unblock folks, with the best split possible, sure, > keeping in mind they'll need to be cleaned

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

hans added a comment. In D83013#2132070 , @echristo wrote: > Adding Chandler and Alina here as well. > > In general, I don't think that this is such a great idea. Being able to have > this sort of thing work more reliably is one of the reasons for the new

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

asbirlea added a comment. In D83013#2132088 , @aeubanks wrote: > In D83013#2132070 , @echristo wrote: > > > Adding Chandler and Alina here as well. > > > > In general, I don't think that this is such a great idea. B

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 275763. zequanwu added a comment. Delete `enable-npm-call-graph-profile` option for NPM, using `enable-call-graph-profile` for both LPM and NPM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https:

Re: [PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

On Sun, Jul 5, 2020 at 8:47 PM Arthur Eubanks via Phabricator < revi...@reviews.llvm.org> wrote: > aeubanks added a comment. > > In D83013#2132070 , @echristo > wrote: > > > Adding Chandler and Alina here as well. > > > > In general, I don't think that this

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

aeubanks added a comment. In D83013#2132070 , @echristo wrote: > Adding Chandler and Alina here as well. > > In general, I don't think that this is such a great idea. Being able to have > this sort of thing work more reliably is one of the reasons for the

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

echristo added reviewers: chandlerc, asbirlea. echristo added a comment. Adding Chandler and Alina here as well. In general, I don't think that this is such a great idea. Being able to have this sort of thing work more reliably is one of the reasons for the new pass manager. I think I'd like to

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added inline comments. Comment at: llvm/tools/opt/opt.cpp:281 +static cl::opt EnableCallGraphProfile( +"enable-call-graph-profile", cl::init(true), cl::Hidden, zhizhouy wrote: > MaskRay wrote: > > zhizhouy wrote: > > > MaskRay wrote: > > > > If ther

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zhizhouy added inline comments. Comment at: llvm/tools/opt/opt.cpp:281 +static cl::opt EnableCallGraphProfile( +"enable-call-graph-profile", cl::init(true), cl::Hidden, MaskRay wrote: > zhizhouy wrote: > > MaskRay wrote: > > > If there is no strong need for

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added inline comments. Comment at: llvm/tools/opt/opt.cpp:281 +static cl::opt EnableCallGraphProfile( +"enable-call-graph-profile", cl::init(true), cl::Hidden, zhizhouy wrote: > MaskRay wrote: > > If there is no strong need for tuning this, please d

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zhizhouy added inline comments. Comment at: llvm/tools/opt/opt.cpp:281 +static cl::opt EnableCallGraphProfile( +"enable-call-graph-profile", cl::init(true), cl::Hidden, MaskRay wrote: > If there is no strong need for tuning this, please delete the option an

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

MaskRay added subscribers: zhizhouy, void. MaskRay added inline comments. Comment at: llvm/tools/opt/opt.cpp:281 +static cl::opt EnableCallGraphProfile( +"enable-call-graph-profile", cl::init(true), cl::Hidden, If there is no strong need for tuning this, pl

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 275558. zequanwu added a comment. Enable CGProfilePass for opt with LPM by default, like opt with NPM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 Files: clang/l

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 275161. zequanwu added a comment. update. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D83013/new/ https://reviews.llvm.org/D83013 Files: clang/lib/CodeGen/BackendUtil.cpp llvm/include/llvm/InitializePass

[PATCH] D83013: [LPM] Port CGProfilePass from NPM to LPM

zequanwu updated this revision to Diff 275160. zequanwu marked 7 inline comments as done. zequanwu added a comment. Herald added a project: clang. Herald added a subscriber: cfe-commits. Address comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org