Page MenuHomePhabricator

[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.