Page MenuHomePhabricator

Add -fdiagnostics-show-hotness
ClosedPublic

Authored by anemet on Aug 8 2016, 3:35 PM.

Details

Summary

I've recently added the ability for optimization remarks to include the
hotness of the corresponding code region. This uses PGO and allows
filtering of the optimization remarks by relevance. The idea was first
discussed here:
http://thread.gmane.org/gmane.comp.compilers.llvm.devel/98334

The general goal is to produce a YAML file with the remarks. Then, an
external tool could dynamically filter these by hotness and perhaps by
other things.

That said it makes sense to also expose this at the more basic level
where we just include the hotness info with each optimization remark.
For example, in D22694, the clang flag was pretty useful to measure the
overhead of the additional analyses required to include hotness.
(Without the flag we don't even run the analyses.)

For the record, Hal has already expressed support for the idea of this
patch on IRC.

Diff Detail

Repository
rL LLVM

Event Timeline

anemet updated this revision to Diff 67234.Aug 8 2016, 3:35 PM
anemet retitled this revision from to Add -Rpass-with-hotness.
anemet updated this object.
anemet added reviewers: hfinkel, rjmccall, aaron.ballman.
anemet added a subscriber: cfe-commits.
aaron.ballman added inline comments.Aug 9 2016, 6:06 AM
include/clang/Basic/DiagnosticDriverKinds.td
186 ↗(On Diff #67234)

Given that there's only one driver option that requires this diagnostic, I would replace the %0 and %1 directives with their hard-coded text for right now (unless you plan on needing this generality in the near future). If we need to generalize the diagnostic in the future, we can do so using a %select directive.

anemet updated this revision to Diff 67379.Aug 9 2016, 11:21 AM

Address Aaron's comment

anemet marked an inline comment as done.
anemet added subscribers: dnovillo, rsmith.

Add more reviewers suggested by Aaron. @dnovillo, @rsmith, is my thinking correct that this new flag does not belong to the R_Group.

anemet updated this revision to Diff 67653.Aug 10 2016, 9:28 PM

Since -Rpass-with-hotness is not part of R_group, we need to manually forward
it to clang -cc1. I've also extended the test to cover this bug.

aaron.ballman accepted this revision.Aug 16 2016, 5:40 AM
aaron.ballman edited edge metadata.

This LGTM, but you should wait for confirmation on the behavior of the R_Group behavior from @dnovillo or @rsmith before committing.

This revision is now accepted and ready to land.Aug 16 2016, 5:40 AM

@dnovillo or @rsmith, can you please confirm if you agree that this new option -Rpass-with-hotness should not be part of R_group. R_group options enable/disable groups of remarks whereas this one is only modifying the text of the remarks. Thanks!

dnovillo edited edge metadata.Aug 26 2016, 9:35 AM

@dnovillo or @rsmith, can you please confirm if you agree that this new option -Rpass-with-hotness should not be part of R_group. R_group options enable/disable groups of remarks whereas this one is only modifying the text of the remarks. Thanks!

I'm fine with it, but I don't have much of a say in how option groups are organized. If Richard agrees, then it LGTM.

anemet added a comment.Sep 6 2016, 9:34 AM

I'm fine with it, but I don't have much of a say in how option groups are organized. If Richard agrees, then it LGTM.

Ping. Would it be OK to commit this with the two LGTMs? I am around to fix any fall-outs or post-commit reviews from Richard.

rsmith edited edge metadata.Sep 6 2016, 10:36 AM

Sorry I missed this until now.

I think this should not be an -R option at all; like -W flags, the idea is for -R to only act as a filter for which diagnostics are shown. This option seems much more closely related to options like -fdiagnostics-show-option and -fdiagnostics-format=, and so should probably have a -fdiagnostics-something name.

I think this should not be an -R option at all; like -W flags, the idea is for -R to only act as a filter for which diagnostics are shown. This option seems much more closely related to options like -fdiagnostics-show-option and -fdiagnostics-format=, and so should probably have a -fdiagnostics-something name.

Sounds fine to me. Any preference from:

-fdiagnostics-include-hotness-in-remarks
-fpass-diagnostics-with-hotness
-fpass-diagnostics-include-hotness

or something else?

rsmith added a comment.Sep 6 2016, 3:00 PM

I think this should not be an -R option at all; like -W flags, the idea is for -R to only act as a filter for which diagnostics are shown. This option seems much more closely related to options like -fdiagnostics-show-option and -fdiagnostics-format=, and so should probably have a -fdiagnostics-something name.

Sounds fine to me. Any preference from:

-fdiagnostics-include-hotness-in-remarks
-fpass-diagnostics-with-hotness
-fpass-diagnostics-include-hotness

or something else?

I think this should start with -fdiagnostics to group it with other similar flags. -fdiagnostics-include-hotness-in-remarks seems too specific to me -- I could imagine generating warnings from the middle-end including hotness information, not just remarks. How about -fdiagnostics-show-hotness? (This would fit nicely with the existing -fdiagnostics-show-* flags.)

anemet added a comment.Sep 6 2016, 3:04 PM

I think this should start with -fdiagnostics to group it with other similar flags. -fdiagnostics-include-hotness-in-remarks seems too specific to me -- I could imagine generating warnings from the middle-end including hotness information, not just remarks. How about -fdiagnostics-show-hotness? (This would fit nicely with the existing -fdiagnostics-show-* flags.)

SGTM. I'll update the patch.

anemet updated this revision to Diff 70502.Sep 6 2016, 11:04 PM
anemet edited edge metadata.

This renames the flag to -fdiagnostics-show-hotness as requested by Richard.
Also added the missing documentation bits.

anemet retitled this revision from Add -Rpass-with-hotness to Add -fdiagnostics-show-hotness.Sep 6 2016, 11:04 PM
anemet updated this object.
rsmith added inline comments.Sep 7 2016, 12:18 AM
lib/Frontend/CompilerInvocation.cpp
844 ↗(On Diff #70502)

You should just use hasArg here, there is no -fno-diagnostics-show-hotness flag for -cc1.

anemet marked an inline comment as done.Sep 7 2016, 9:21 AM
anemet updated this revision to Diff 70547.Sep 7 2016, 9:22 AM
anemet updated this object.

Address Richard's comment

rsmith accepted this revision.Sep 7 2016, 11:00 AM
rsmith edited edge metadata.
This revision was automatically updated to reflect the committed changes.