[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 <danielcdh at gmail.com> 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 <dblaikie at gmail.com> 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
> > <cfe-commits at lists.llvm.org> 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 at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20170711/cfa58487/attachment.html>