Re: [PATCH] D63155: [clang][NewPM] Fix broken profile test

On Fri, Jun 28, 2019 at 5:28 PM Xinliang David Li wrote: > I agree that the test coverage needs to be improved eg better check etc. > PGO is rarely run under -O0. But I agree that we should improve the test. I sent out https://reviews.llvm.org/D64029. Chanlder: could you take a look? Thanks,

Re: [PATCH] D63155: [clang][NewPM] Fix broken profile test

I agree that the test coverage needs to be improved eg better check etc. David On Fri, Jun 28, 2019 at 5:21 PM Chandler Carruth via Phabricator via llvm-commits wrote: > chandlerc added a comment. > > In D63155#1563275 , @xur wrote: > > > In D63155#156

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc added a comment. In D63155#1563275 , @xur wrote: > In D63155#1563266 , @chandlerc wrote: > > > I just think we also need to understand why *no other test failed*, and why > > the only test we have for doi

[PATCH] D63155: [clang][NewPM] Fix broken profile test

leonardchan added a comment. Reverted in r364692 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63155/new/ https://reviews.llvm.org/D63155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/

[PATCH] D63155: [clang][NewPM] Fix broken profile test

xur added a comment. In D63155#1563266 , @chandlerc wrote: > In D63155#1563240 , @leonardchan > wrote: > > > In D63155#1563229 , @xur wrote: > > > > > This patch does not ma

[PATCH] D63155: [clang][NewPM] Fix broken profile test

xur added a comment. they are not doing the exacly the same thing for old pass manager and new pass manger: old pass manger is doing instrumentation, while the new pass manager with this change is NOT. the test is not check instrumentation, (it only check the variables that generates by InstroP

[PATCH] D63155: [clang][NewPM] Fix broken profile test

leonardchan added a comment. Understood. I'll revert this patch. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63155/new/ https://reviews.llvm.org/D63155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:/

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc added a comment. In D63155#1563240 , @leonardchan wrote: > In D63155#1563229 , @xur wrote: > > > This patch does not make sense to me. > > > > The reason of failing with -fexperimental-new-pass-manager is

Re: [PATCH] D63155: [clang][NewPM] Fix broken profile test

they are not doing the exacly the same thing for old pass manager and new pass manger: old pass manger is doing instrumentation, while the new pass manager with this change is NOT. the test is not check instrumentation, (it only check the variables that generates by InstroProfiling pass). In this s

[PATCH] D63155: [clang][NewPM] Fix broken profile test

leonardchan added a comment. > I mean, I'm happy for the patch to be reverted, but I still really don't > understand why this fixes the test to work *exactly* the same as w/ the old > pass manager and doesn't break any other tests if it is completely wrong? It > seems like there must be somethi

[PATCH] D63155: [clang][NewPM] Fix broken profile test

leonardchan added a comment. In D63155#1563229 , @xur wrote: > This patch does not make sense to me. > > The reason of failing with -fexperimental-new-pass-manager is because we > don't do PGO instrumentation at -O0. This is due to the fact that PGO > in

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc added a comment. In D63155#1563229 , @xur wrote: > This patch does not make sense to me. > > The reason of failing with -fexperimental-new-pass-manager is because we > don't do PGO instrumentation at -O0. This is due to the fact that PGO > inst

[PATCH] D63155: [clang][NewPM] Fix broken profile test

xur added a comment. This patch does not make sense to me. The reason of failing with -fexperimental-new-pass-manager is because we don't do PGO instrumentation at -O0. This is due to the fact that PGO instrumentation/use passes are under PassBuilder::buildPerModuleDefaultPipeline. This patch

[PATCH] D63155: [clang][NewPM] Fix broken profile test

davidxl added a comment. Rong, can you take a look at this patch? Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63155/new/ https://reviews.llvm.org/D63155 ___ cfe-commits mailing list cfe-commits@lists.llvm.org https:

[PATCH] D63155: [clang][NewPM] Fix broken profile test

This revision was automatically updated to reflect the committed changes. Closed by commit rL363278: [clang][NewPM] Fix broken profile test (authored by leonardchan, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.l

[PATCH] D63155: [clang][NewPM] Fix broken profile test

chandlerc accepted this revision. chandlerc added a comment. This revision is now accepted and ready to land. Let's update the test to explicitly run w/ both PMs to make sure this keeps working. LGTM with that change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://re

[PATCH] D63155: [clang][NewPM] Fix broken profile test

leonardchan created this revision. leonardchan added reviewers: chandlerc, echristo, phosek, serge-sans-paille. leonardchan added a project: clang. leonardchan added a parent revision: D62225: [clang][NewPM] Fixing -O0 tests that are broken under new PM. This contains the part of D62225