Page MenuHomePhabricator

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

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

Unit TestsFailed

TimeTest
400 mslinux > HWAddressSanitizer-x86_64.TestCases::sizes.cpp
Script: -- : 'RUN: at line 3'; /mnt/disks/ssd0/agent/llvm-project/build/./bin/clang --driver-mode=g++ -m64 -gline-tables-only -fsanitize=hwaddress -fuse-ld=lld -mcmodel=large -mllvm -hwasan-globals -mllvm -hwasan-use-short-granules -mllvm -hwasan-instrument-landing-pads=0 -mllvm -hwasan-instrument-personality-functions /mnt/disks/ssd0/agent/llvm-project/compiler-rt/test/hwasan/TestCases/sizes.cpp -nostdlib++ -lstdc++ -o /mnt/disks/ssd0/agent/llvm-project/build/projects/compiler-rt/test/hwasan/X86_64/TestCases/Output/sizes.cpp.tmp
40 mslinux > LLVM.CodeGen/AArch64::O3-pipeline.ll
Script: -- : 'RUN: at line 1'; /mnt/disks/ssd0/agent/llvm-project/build/bin/llc --debugify-and-strip-all-safe=0 -mtriple=arm64-- -O3 -debug-pass=Structure < /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/O3-pipeline.ll -o /dev/null 2>&1 | grep -v "Verify generated machine code" | /mnt/disks/ssd0/agent/llvm-project/build/bin/FileCheck /mnt/disks/ssd0/agent/llvm-project/llvm/test/CodeGen/AArch64/O3-pipeline.ll
110 mswindows > LLVM.CodeGen/AArch64::O3-pipeline.ll
Script: -- : 'RUN: at line 1'; c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\llc.exe --debugify-and-strip-all-safe=0 -mtriple=arm64-- -O3 -debug-pass=Structure < C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\CodeGen\AArch64\O3-pipeline.ll -o /dev/null 2>&1 | grep -v "Verify generated machine code" | c:\ws\w16n2-1\llvm-project\premerge-checks\build\bin\filecheck.exe C:\ws\w16n2-1\llvm-project\premerge-checks\llvm\test\CodeGen\AArch64\O3-pipeline.ll

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 ↗(On Diff #284966)

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 ↗(On Diff #284966)

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 ↗(On Diff #284966)

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.Tue, Nov 17, 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.Wed, Nov 18, 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.