<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Mon, Jul 10, 2017 at 10:58 AM Dehao Chen <<a href="mailto:danielcdh@gmail.com">danielcdh@gmail.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">This test was originally added in <a href="https://reviews.llvm.org/D34721" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34721</a> with<br>
clang change. It's kind of dup of the previous test<br>
(-check-prefix=SAMPLEPGO) in terms of testing the clang bits. But we<br>
want to make sure the new PM has the expected behavior.</blockquote><div><br>Is the new PM behavior tested in LLVM tests?<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> I guess it<br>
would be acceptable to remove one of them, but I'd prefer to remove<br>
the one with legacy PM. Let me know if you think I should do that.<br></blockquote><div><br>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))<br><br>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.<br><br>Anyway, whatever you think's best - just my 2c & not sure I'm describing it so well.<br><br>- Dave<br> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
Thanks,<br>
Dehao<br>
<br>
On Mon, Jul 10, 2017 at 9:45 AM, David Blaikie <<a href="mailto:dblaikie@gmail.com" target="_blank">dblaikie@gmail.com</a>> wrote:<br>
> Does this test any code in Clang? (given the test is being committed without<br>
> any change in Clang, I'm guessing maybe not) - perhaps this test doesn't<br>
> belong in Clang's test suite?<br>
><br>
> Looks like changes/functionality in LTOPreLinkDefaultPipeline are tested in<br>
> test/Other/new-pm-thinlto-defaults.ll at least?<br>
><br>
> On Fri, Jun 30, 2017 at 10:05 AM Dehao Chen via Phabricator via cfe-commits<br>
> <<a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a>> wrote:<br>
>><br>
>> danielcdh created this revision.<br>
>> Herald added subscribers: eraman, inglorion, mehdi_amini, sanjoy.<br>
>><br>
>> This patch should be enabled after <a href="https://reviews.llvm.org/D34895" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34895</a><br>
>><br>
>><br>
>> <a href="https://reviews.llvm.org/D34896" rel="noreferrer" target="_blank">https://reviews.llvm.org/D34896</a><br>
>><br>
>> Files:<br>
>>   test/CodeGen/pgo-sample-thinlto-summary.c<br>
>><br>
>><br>
>> Index: test/CodeGen/pgo-sample-thinlto-summary.c<br>
>> ===================================================================<br>
>> --- test/CodeGen/pgo-sample-thinlto-summary.c<br>
>> +++ test/CodeGen/pgo-sample-thinlto-summary.c<br>
>> @@ -1,9 +1,7 @@<br>
>>  // RUN: %clang_cc1 -O2<br>
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm<br>
>> -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO<br>
>>  // RUN: %clang_cc1 -O2<br>
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm<br>
>> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO<br>
>>  // RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager<br>
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm<br>
>> -o - 2>&1 | FileCheck %s -check-prefix=SAMPLEPGO<br>
>> -// FIXME: Run the following command once LTOPreLinkDefaultPipeline is<br>
>> -//        customized.<br>
>> -// %clang_cc1 -O2 -fexperimental-new-pass-manager<br>
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm<br>
>> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO<br>
>> +// RUN: %clang_cc1 -O2 -fexperimental-new-pass-manager<br>
>> -fprofile-sample-use=%S/Inputs/pgo-sample-thinlto-summary.prof %s -emit-llvm<br>
>> -flto=thin -o - 2>&1 | FileCheck %s -check-prefix=THINLTO<br>
>>  // Checks if hot call is inlined by normal compile, but not inlined by<br>
>>  // thinlto compile.<br>
>><br>
>><br>
>><br>
>> _______________________________________________<br>
>> cfe-commits mailing list<br>
>> <a href="mailto:cfe-commits@lists.llvm.org" target="_blank">cfe-commits@lists.llvm.org</a><br>
>> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits</a><br>
</blockquote></div></div>