This is an archive of the discontinued LLVM Phabricator instance.

[ThinLTO] Add additional ThinLTO pipeline testing with new PM
ClosedPublic

Authored by tejohnson on Jan 10 2020, 1:35 PM.

Details

Summary

I've added some more extensive ThinLTO pipeline testing with the new PM,
motivated by the bug fixed in D72386.

I beefed up llvm/test/Other/new-pm-pgo.ll a little so that it tests
ThinLTO pre and post link with PGO, similar to the testing for the
default pipelines with PGO.

Added new pre and post link PGO tests for both instrumentation and
sample PGO that exhaustively test the pipelines at different
optimization levels via opt.

Added a clang test to exhaustively test the post link pipeline invoked for
distributed builds. I am currently only testing O2 and O3 since these
are the most important for performance.

It would be nice to add similar exhaustive testing for full LTO, and for
the old PM, but I don't have the bandwidth now and this is a start to
cover some of the situations that are not currently default and were
under tested.

Diff Detail

Event Timeline

tejohnson created this revision.Jan 10 2020, 1:35 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 10 2020, 1:35 PM
tejohnson updated this revision to Diff 237427.Jan 10 2020, 1:43 PM

Remove some cruft

wmi added a comment.Jan 10 2020, 2:52 PM

The additional pipeline testing will catch any future pass change to the pipeline. A related but separate question is do we have a way to check whether there is any other missing pass in thinlto newpm similar as that in D72386?

clang/test/CodeGen/thinlto-distributed-newpm.ll
12

Can we test %clang intead of %clang_cc1? so we don't have to add flags like -vectorize-slp and -vectorize-loops. clang O2/O3 pipeline is something we care about. Those flags added by clang to cc1 options may be subject to change.

tejohnson marked 2 inline comments as done.Jan 10 2020, 4:48 PM
In D72538#1815074, @wmi wrote:

The additional pipeline testing will catch any future pass change to the pipeline. A related but separate question is do we have a way to check whether there is any other missing pass in thinlto newpm similar as that in D72386?

Unfortunately not. That will take some painstaking comparisons between the pass debug output between the old and new PMs, which is going to be manual given the different format and structuring of the dependences. I am going to try to do that at least for a few important cases. As you noted, this is to help flag any future changes (although the last one did actually result in a test being changed, but it looks like no one realized the implications). Hopefully by having additional testing it will be more likely to trigger concerns if it happens again.

clang/test/CodeGen/thinlto-distributed-newpm.ll
12

Good idea, done

tejohnson updated this revision to Diff 237462.Jan 10 2020, 4:48 PM
tejohnson marked an inline comment as done.

Implement suggestion.

wmi accepted this revision.Jan 10 2020, 7:36 PM

LGTM.

This revision is now accepted and ready to land.Jan 10 2020, 7:36 PM
This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Apr 24 2023, 12:54 AM

Would it be possible to cut down the Clang side tests to only check parts that Clang controls in some way, e.g. that SLPVectorizer is enabled? This test is something of a PITA because it tests LLVM implementation details but is not part of the LLVM tests, so it usually gets missed when doing pipeline changes.

Would it be possible to cut down the Clang side tests to only check parts that Clang controls in some way, e.g. that SLPVectorizer is enabled? This test is something of a PITA because it tests LLVM implementation details but is not part of the LLVM tests, so it usually gets missed when doing pipeline changes.

I think we can cut this down significantly, since the llvm tests added here check the full ThinLTO default pipeline setup at different opt levels. However, the point of the clang test is to make sure that for a distributed ThinLTO backend we correctly invoke thinBackend() which should set up the ThinLTO default pipeline. So how about I cut this down to check a single pass that is only invoked in the LTO backends? Specifically, I'm thinking of WholeProgramDevirtPass.

Would it be possible to cut down the Clang side tests to only check parts that Clang controls in some way, e.g. that SLPVectorizer is enabled? This test is something of a PITA because it tests LLVM implementation details but is not part of the LLVM tests, so it usually gets missed when doing pipeline changes.

I think we can cut this down significantly, since the llvm tests added here check the full ThinLTO default pipeline setup at different opt levels. However, the point of the clang test is to make sure that for a distributed ThinLTO backend we correctly invoke thinBackend() which should set up the ThinLTO default pipeline. So how about I cut this down to check a single pass that is only invoked in the LTO backends? Specifically, I'm thinking of WholeProgramDevirtPass.

That sounds reasonable to me!

Would it be possible to cut down the Clang side tests to only check parts that Clang controls in some way, e.g. that SLPVectorizer is enabled? This test is something of a PITA because it tests LLVM implementation details but is not part of the LLVM tests, so it usually gets missed when doing pipeline changes.

I think we can cut this down significantly, since the llvm tests added here check the full ThinLTO default pipeline setup at different opt levels. However, the point of the clang test is to make sure that for a distributed ThinLTO backend we correctly invoke thinBackend() which should set up the ThinLTO default pipeline. So how about I cut this down to check a single pass that is only invoked in the LTO backends? Specifically, I'm thinking of WholeProgramDevirtPass.

That sounds reasonable to me!

Done in e5b0276dc882f7c5b2a349e2f02abf16b1d41322