Re: [PATCH] D34896: Enable the new PM + SamlePGO + ThinLTO testing.

[David Blaikie via cfe-commits]

On Mon, Jul 10, 2017 at 10:58 AM Dehao Chen  wrote:

> This test was originally added in https://reviews.llvm.org/D34721 with
> clang change. It's kind of dup of the previous test
> (-check-prefix=SAMPLEPGO) in terms of testing the clang bits. But we
> want to make sure the new PM has the expected behavior.


Is the new PM behavior tested in LLVM tests?


> I guess it
> would be acceptable to remove one of them, but I'd prefer to remove
> the one with legacy PM. Let me know if you think I should do that.
>

Generally I'd leave it as-is (before this change) (my mental model/approach
is that changing/updating this test is like, for example, changing/adding
another test for C++ lambdas when/after a new implementation of
(fictional/notional) mem2reg was added - generally testing all these
combinations isn't practical, so as long as the pieces of functionality are
tested, that tends to be as far as it goes in the LLVM project (granted
there are a few places that are tricky - like these non-IR, purely
API-based, relationships between Clang & LLVM, where something approaching
an end-to-end test may be appropriate))

Not sure how other folks feel about such things & guess removing the old
test/upgrading it to test the experimental pass manager doesn't make it any
worse than the original test from my perspective. But it still feels like
it's not quite the right philosophy - if adding the test to cover the new
pass manager is important, then losing the coverage on the old scenario is
problematic... - if it isn't, then there's no need to add the new test
coverage at all & the test should remain as it was.

Anyway, whatever you think's best - just my 2c & not sure I'm describing it
so well.

- Dave


>
> Thanks,
> Dehao
>
> On Mon, Jul 10, 2017 at 9:45 AM, David Blaikie  wrote:
> > Does this test any code in Clang? (given the test is being committed
> without
> > any change in Clang, I'm guessing maybe not) - perhaps this test doesn't
> > belong in Clang's test suite?
> >
> > Looks like changes/functionality in LTOPreLinkDefaultPipeline are tested
> in
> > test/Other/new-pm-thinlto-defaults.ll at least?
> >
> > On Fri, Jun 30, 2017 at 10:05 AM Dehao Chen via Phabricator via
> cfe-commits
> >  wrote:
> >>
> >> danielcdh created this revision.
> >> Herald added subscribers: eraman, inglorion, mehdi_amini, sanjoy.
> >>
> >> This patch should be enabled after https://reviews.llvm.org/D34895
> >>
> >>
> >> https://reviews.llvm.org/D34896
> >>
> >> Files:
> >>   test/CodeGen/pgo-sample-thinlto-summary.c
> >>
> >>
> >> Index: test/CodeGen/pgo-sample-thinlto-summary.c
> >> ===
> >> --- test/CodeGen/pgo-sample-thinlto-summary.c
> >> +++ test/CodeGen/pgo-sample-thinlto-summary.c
> >> @@ -1,9 +1,7 @@
> >>  // RUN: %clang_cc1 -O2
> >> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm
> >> -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
> >>  // RUN: %clang_cc1 -O2
> >> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm
> >> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
> >>  // RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager
> >> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm
> >> -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
> >> -// FIXME: Run the following command once LTOPreLinkDefaultPipeline is
> >> -//customized.
> >> -// %clang_cc1 -O2 -fexperimental-new-pass-manager
> >> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm
> >> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
> >> +// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager
> >> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm
> >> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
> >>  // Checks if hot call is inlined by normal compile, but not inlined by
> >>  // thinlto compile.
> >>
> >>
> >>
> >> ___
> >> cfe-commits mailing list
> >> cfe-commits@lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D34896: Enable the new PM + SamlePGO + ThinLTO testing.

[Dehao Chen via cfe-commits]

This test was originally added in https://reviews.llvm.org/D34721 with
clang change. It's kind of dup of the previous test
(-check-prefix=SAMPLEPGO) in terms of testing the clang bits. But we
want to make sure the new PM has the expected behavior. I guess it
would be acceptable to remove one of them, but I'd prefer to remove
the one with legacy PM. Let me know if you think I should do that.

Thanks,
Dehao

On Mon, Jul 10, 2017 at 9:45 AM, David Blaikie  wrote:
> Does this test any code in Clang? (given the test is being committed without
> any change in Clang, I'm guessing maybe not) - perhaps this test doesn't
> belong in Clang's test suite?
>
> Looks like changes/functionality in LTOPreLinkDefaultPipeline are tested in
> test/Other/new-pm-thinlto-defaults.ll at least?
>
> On Fri, Jun 30, 2017 at 10:05 AM Dehao Chen via Phabricator via cfe-commits
>  wrote:
>>
>> danielcdh created this revision.
>> Herald added subscribers: eraman, inglorion, mehdi_amini, sanjoy.
>>
>> This patch should be enabled after https://reviews.llvm.org/D34895
>>
>>
>> https://reviews.llvm.org/D34896
>>
>> Files:
>>   test/CodeGen/pgo-sample-thinlto-summary.c
>>
>>
>> Index: test/CodeGen/pgo-sample-thinlto-summary.c
>> ===
>> --- test/CodeGen/pgo-sample-thinlto-summary.c
>> +++ test/CodeGen/pgo-sample-thinlto-summary.c
>> @@ -1,9 +1,7 @@
>>  // RUN: %clang_cc1 -O2
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm
>> -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
>>  // RUN: %clang_cc1 -O2
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm
>> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
>>  // RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm
>> -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
>> -// FIXME: Run the following command once LTOPreLinkDefaultPipeline is
>> -//customized.
>> -// %clang_cc1 -O2 -fexperimental-new-pass-manager
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm
>> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
>> +// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm
>> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
>>  // Checks if hot call is inlined by normal compile, but not inlined by
>>  // thinlto compile.
>>
>>
>>
>> ___
>> cfe-commits mailing list
>> cfe-commits@lists.llvm.org
>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D34896: Enable the new PM + SamlePGO + ThinLTO testing.

[David Blaikie via cfe-commits]

Does this test any code in Clang? (given the test is being committed
without any change in Clang, I'm guessing maybe not) - perhaps this test
doesn't belong in Clang's test suite?

Looks like changes/functionality in LTOPreLinkDefaultPipeline are tested in
test/Other/new-pm-thinlto-defaults.ll at least?

On Fri, Jun 30, 2017 at 10:05 AM Dehao Chen via Phabricator via cfe-commits
 wrote:

> danielcdh created this revision.
> Herald added subscribers: eraman, inglorion, mehdi_amini, sanjoy.
>
> This patch should be enabled after https://reviews.llvm.org/D34895
>
>
> https://reviews.llvm.org/D34896
>
> Files:
>   test/CodeGen/pgo-sample-thinlto-summary.c
>
>
> Index: test/CodeGen/pgo-sample-thinlto-summary.c
> ===
> --- test/CodeGen/pgo-sample-thinlto-summary.c
> +++ test/CodeGen/pgo-sample-thinlto-summary.c
> @@ -1,9 +1,7 @@
>  // RUN: %clang_cc1 -O2
> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
>  // RUN: %clang_cc1 -O2
> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
>  // RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager
> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
> -// FIXME: Run the following command once LTOPreLinkDefaultPipeline is
> -//customized.
> -// %clang_cc1 -O2 -fexperimental-new-pass-manager
> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
> +// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager
> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s
> -emit-llvm -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
>  // Checks if hot call is inlined by normal compile, but not inlined by
>  // thinlto compile.
>
>
>
> ___
> cfe-commits mailing list
> cfe-commits@lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34896: Enable the new PM + SamlePGO + ThinLTO testing.

[Teresa Johnson via Phabricator via cfe-commits]

tejohnson accepted this revision.
tejohnson added a comment.
This revision is now accepted and ready to land.

LGTM


https://reviews.llvm.org/D34896



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D34896: Enable the new PM + SamlePGO + ThinLTO testing.

[Dehao Chen via Phabricator via cfe-commits]

danielcdh created this revision.
Herald added subscribers: eraman, inglorion, mehdi_amini, sanjoy.

This patch should be enabled after https://reviews.llvm.org/D34895


https://reviews.llvm.org/D34896

Files:
  test/CodeGen/pgo-sample-thinlto-summary.c


Index: test/CodeGen/pgo-sample-thinlto-summary.c
===
--- test/CodeGen/pgo-sample-thinlto-summary.c
+++ test/CodeGen/pgo-sample-thinlto-summary.c
@@ -1,9 +1,7 @@
 // RUN: %clang_cc1 -O2 
-fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -o 
- 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
 // RUN: %clang_cc1 -O2 
-fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm 
-flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
 // RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager 
-fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -o 
- 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
-// FIXME: Run the following command once LTOPreLinkDefaultPipeline is
-//customized.
-// %clang_cc1 -O2 -fexperimental-new-pass-manager 
-fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm 
-flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
+// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager 
-fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm 
-flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
 // Checks if hot call is inlined by normal compile, but not inlined by
 // thinlto compile.
 


Index: test/CodeGen/pgo-sample-thinlto-summary.c
===
--- test/CodeGen/pgo-sample-thinlto-summary.c
+++ test/CodeGen/pgo-sample-thinlto-summary.c
@@ -1,9 +1,7 @@
 // RUN: %clang_cc1 -O2 -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
 // RUN: %clang_cc1 -O2 -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
 // RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO
-// FIXME: Run the following command once LTOPreLinkDefaultPipeline is
-//customized.
-// %clang_cc1 -O2 -fexperimental-new-pass-manager -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
+// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO
 // Checks if hot call is inlined by normal compile, but not inlined by
 // thinlto compile.
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits