Page MenuHomePhabricator

[Remarks][2/2] Expand remarks hotness threshold option support in more tools
ClosedPublic

Authored by weiwang on Aug 11 2020, 10:37 PM.

Details

Summary

This is the #2 of 2 changes that make remarks hotness threshold option
available in more tools. The changes also allow the threshold to sync with
hotness threshold from profile summary with special value 'auto'.

This change expands remarks hotness threshold option
-fdiagnostics-hotness-threshold in clang and *-remarks-hotness-threshold in
other tools to utilize hotness threshold from profile summary.

Remarks hotness filtering relies on several driver options. Table below lists
how different options are correlated and affect final remarks outputs:

| profile | hotness | threshold | remarks printed |
|---------|---------|-----------|-----------------|
| No      | No      | No        | All             |
| No      | No      | Yes       | None            |
| No      | Yes     | No        | All             |
| No      | Yes     | Yes       | None            |
| Yes     | No      | No        | All             |
| Yes     | No      | Yes       | None            |
| Yes     | Yes     | No        | All             |
| Yes     | Yes     | Yes       | >=threshold     |

In the presence of profile summary, it is often more desirable to directly use
the hotness threshold from profile summary. The new argument value 'auto'
indicates threshold will be synced with hotness threshold from profile summary
during compilation. The "auto" threshold relies on the availability of profile
summary. In case of missing such information, no remarks will be generated.

Diff Detail

Event Timeline

weiwang created this revision.Aug 11 2020, 10:37 PM
Herald added a reviewer: MaskRay. · View Herald Transcript
Herald added projects: Restricted Project, Restricted Project. · View Herald Transcript
weiwang requested review of this revision.Aug 11 2020, 10:37 PM
weiwang added reviewers: wenlei, hoyFB.

This is the 3rd of 3 dependent patches:

  1. [lld] Enable remarks hotness filtering in lld: https://reviews.llvm.org/D85809
  2. [clang] Pass-through remarks options to lld: https://reviews.llvm.org/D85810
  3. [remarks] Optimization remarks hotness filtering from profile summary: https://reviews.llvm.org/D85808
weiwang retitled this revision from [remarks] Optimization remarks hotness filetering from profile summary to [remarks] Optimization remarks hotness filtering from profile summary.Aug 11 2020, 10:53 PM

This is the 3rd of 3 dependent patches:

  1. [lld] Enable remarks hotness filtering in lld: https://reviews.llvm.org/D85809
  2. [clang] Pass-through remarks options to lld: https://reviews.llvm.org/D85810
  3. [remarks] Optimization remarks hotness filtering from profile summary: https://reviews.llvm.org/D85808

You can relate these as Parent/Child patches in Phabricator. See "Edit Related Revisions" on the top right. It helps reviewers to see the relationship and keep track of what patches are still open.

Note these relationships get set up automatically when you upload a patch with "Depends on Dxxxxx." in the description.

lld/ELF/Config.h
280

Since this field is being added in patch D85809 as an unsigned, why not add it as llvm::Optional<> there to start with instead of adding and then changing?

Ditto for a number of other places in this patch.

This is the 3rd of 3 dependent patches:

  1. [lld] Enable remarks hotness filtering in lld: https://reviews.llvm.org/D85809
  2. [clang] Pass-through remarks options to lld: https://reviews.llvm.org/D85810
  3. [remarks] Optimization remarks hotness filtering from profile summary: https://reviews.llvm.org/D85808

You can relate these as Parent/Child patches in Phabricator. See "Edit Related Revisions" on the top right. It helps reviewers to see the relationship and keep track of what patches are still open.

Note these relationships get set up automatically when you upload a patch with "Depends on Dxxxxx." in the description.

Thanks for the tip.

lld/ELF/Config.h
280

Thanks for the feedback! It does seem awkward. When doing the splitting, I tried to make the 3 patches look more self-contained from each other. The Optional type change seems unrelated with the first patch.

tejohnson added inline comments.Aug 13 2020, 5:19 PM
lld/ELF/Config.h
280

I would recommend going straight to Optional in the first patch, and just note there that the type is needed for the follow on patch. That will avoid unnecessary churn.

weiwang updated this revision to Diff 286203.Aug 17 2020, 10:53 PM

update:

  1. reorganize code splitting with parent diff.
  2. format and style change.
weiwang retitled this revision from [remarks] Optimization remarks hotness filtering from profile summary to [Remarks][2/2] Expand remarks hotness threshold option support in more tools.Aug 17 2020, 10:55 PM
weiwang edited the summary of this revision. (Show Details)
tejohnson added inline comments.Aug 25 2020, 11:30 AM
llvm/lib/Analysis/OptimizationRemarkEmitter.cpp
103–110

Can you clarify what you mean by this only happening once?

weiwang added inline comments.Aug 27 2020, 1:24 PM
llvm/lib/Analysis/OptimizationRemarkEmitter.cpp
103–110

The check of Context.isDiagnosticsHotnessThresholdSetFromPSI() guarantees that ProfileSummaryInfoWrapperPass and setDiagnosticsHotnessThreshold would only be called once. Once the threshold is set from PSI, the check always returns false. But strictly speaking, since PSI is immutable, it only happens once with/without the check.

@tejohnson @MaskRay Do you have other comments to the change?

Largely LGTM, except that the clang changes are missing a corresponding test.

weiwang updated this revision to Diff 305891.Nov 17 2020, 1:19 PM

update test case for clang option pass-through

Thanks for adding the Driver test. I was thinking of something to test the CompilerInvocation changes, similar to your test using opt, that ensures the option has the desired behavior when invoked via clang. Looks like there is an existing test clang/test/Frontend/optimization-remark-with-hotness.c that perhaps could be extended or leveraged?

weiwang updated this revision to Diff 306256.Nov 18 2020, 4:23 PM
  1. Add clang test with remarks output.
  2. Fix a missing dependency on PSI in legacy pass manager.

Thanks for adding the Driver test. I was thinking of something to test the CompilerInvocation changes, similar to your test using opt, that ensures the option has the desired behavior when invoked via clang. Looks like there is an existing test clang/test/Frontend/optimization-remark-with-hotness.c that perhaps could be extended or leveraged?

Thanks for the suggestion. I have added a new test case for clang. The source file used is the same one in the opt test case.

@tejohnson @MaskRay Do you have other comments?

tejohnson accepted this revision.Nov 30 2020, 8:20 PM

lgtm with a couple of minor nits noted below that you can fix before submitting

clang/include/clang/Basic/CodeGenOptions.h
344

nit: s/emarks/remarks/

349

nit: s/threhold/threshold/

clang/test/Frontend/remarks-hotness.cpp
27 ↗(On Diff #308433)

nit: move this down by the corresponding REMARKS line below? (makes the difference between the 2 cases clearer)

This revision is now accepted and ready to land.Nov 30 2020, 8:20 PM
weiwang updated this revision to Diff 308536.Nov 30 2020, 9:51 PM
  1. Fix typo.
  2. Minor order adjustment in testcase.
weiwang marked 3 inline comments as done.Nov 30 2020, 9:52 PM

lgtm with a couple of minor nits noted below that you can fix before submitting

Thanks for pointing them out. Really appreciate it!

This revision was landed with ongoing or failed builds.Nov 30 2020, 9:57 PM
This revision was automatically updated to reflect the committed changes.

I'm seeing build failures that might be triggered by this patch: http://lab.llvm.org:8011/#/builders/109/builds/3714

hans added a subscriber: hans.Dec 1 2020, 2:51 AM

I'm seeing build failures that might be triggered by this patch: http://lab.llvm.org:8011/#/builders/109/builds/3714

It seems to affect a lot of the main bots (random example: http://lab.llvm.org:8011/#/builders/52/builds/1963)
And here's another one: http://45.33.8.238/linux/34102/step_12.txt
We also hit this in Chromium.

I think David just fixed it in https://github.com/llvm/llvm-project/commit/09d82fa95f4561a6a2ce80bce00209018ba70c24

Ah, I am sorry. Thanks for fixing it.