This is an archive of the discontinued LLVM Phabricator instance.

[test-suite] Save stats for LTO step too.
ClosedPublic

Authored by fhahn on Apr 12 2018, 1:35 AM.

Details

Summary

Currently we do not generate stats for the LTO step. This change
together with D45531 allows us to gather stats for the LTO step too.

Diff Detail

Event Timeline

fhahn created this revision.Apr 12 2018, 1:35 AM
  • Do non gold linkers all accept the -Wl,-plugin-opt lines?
  • Maybe the clang driver knows if a gold linker is used and could add the -Wl,-plugin opt lines itself when -save-stats is used?
fhahn updated this revision to Diff 142942.Apr 18 2018, 8:08 AM
fhahn added a subscriber: tejohnson.

Thanks for having a look! After @tejohnson 's feedback, I've updated the llvm patch and added a clang patch that adds support for -save-stats with LTO to the clang driver.

fhahn added a comment.Apr 25 2018, 6:13 AM

Ping. The related patches clang/llvm patches have been merged. Clang now accepts and passes on -save-stats= when doing linking + LTO with the gold plugin.

rengolin accepted this revision.Apr 25 2018, 6:16 AM

LGTM, thanks!

This revision is now accepted and ready to land.Apr 25 2018, 6:16 AM
This revision was automatically updated to reflect the committed changes.
MatzeB added inline comments.Apr 25 2018, 8:59 AM
test-suite/trunk/CMakeLists.txt
198 ↗(On Diff #143946)

You could just append the flag to LDFLAHS unconditionally now, can you? (Or do nothing at all, I think currently we append cflags to all linker invocations too; which admittedly is bad style but unlikely to change...

fhahn added inline comments.Apr 26 2018, 3:42 PM
test-suite/trunk/CMakeLists.txt
198 ↗(On Diff #143946)

Ah I saw your comment too late, sorry! I think the main reason I originally added it conditionally is to avoid "unused argument" warnings when not doing LTO. What do you think?

MatzeB added inline comments.Apr 26 2018, 4:26 PM
test-suite/trunk/CMakeLists.txt
198 ↗(On Diff #143946)

I only added the comments after you committed so no worries. But I still think that adding all this after you did the changes for working without -Wl,plugin are not necessary anymore, maybe try removing them and if things still work remove this?

fhahn added inline comments.May 4 2018, 10:55 AM
test-suite/trunk/CMakeLists.txt
198 ↗(On Diff #143946)

I just tried, unconditionally adding -save-stats to the LDFLAGS works. But for non-lto builds, it produces argument unused during compilation warnings for each linker invocation. IMO it would be slightly better to only add it for LTO, to avoid those warnings in the non-LTO case for now.