Page MenuHomePhabricator

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

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

Details

Summary

This is the #1 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 modifies the interface of lto::setupLLVMOptimizationRemarks() to
accept remarks hotness threshold. Update all the tools that use it with remarks
hotness threshold options:

  • lld: '--opt-remarks-hotness-threshold='
  • llvm-lto2: '--pass-remarks-hotness-threshold='
  • llvm-lto: '--lto-pass-remarks-hotness-threshold='
  • gold plugin: '-plugin-opt=opt-remarks-hotness-threshold='
NOTE: Internal remarks hotness threshold type is updated to accommodate the next change. The 'auto' value is allowed, but has no effect.

Diff Detail

Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

update:
add remarks hotness threshold to all the tools uses lto::setupLLVMOptimizationRemarks

weiwang retitled this revision from [lld] Enable remarks hotness filtering in lld and llvm-lto2 to [LTO] Make setupLLVMOptimizationRemarks accept remarks hotness threshold.Aug 17 2020, 1:00 PM
weiwang edited the summary of this revision. (Show Details)
weiwang updated this revision to Diff 286121.Aug 17 2020, 1:07 PM
weiwang marked an inline comment as done.

format update

weiwang updated this revision to Diff 286122.Aug 17 2020, 1:08 PM
weiwang marked an inline comment as done.

format update

weiwang marked 2 inline comments as done.Aug 17 2020, 1:09 PM
weiwang edited the summary of this revision. (Show Details)Aug 17 2020, 1:24 PM
MaskRay added inline comments.Aug 17 2020, 2:24 PM
lld/ELF/Config.h
268

This should be moved above

llvm::StringRef optRemarksFilename;
llvm::StringRef optRemarksPasses;
llvm::StringRef optRemarksFormat;
lld/ELF/Options.td
553

<integer> might be preciser.

llvm/tools/gold/gold-plugin.cpp
303

Drop unneeded inner braces

943

stray ;

MaskRay requested changes to this revision.Aug 17 2020, 2:24 PM
This revision now requires changes to proceed.Aug 17 2020, 2:24 PM
weiwang added inline comments.Aug 17 2020, 3:34 PM
lld/ELF/Config.h
268

Thanks for the comments! I will upload another revision to fix this, and also address Teresa's comment in https://reviews.llvm.org/D85808.

weiwang updated this revision to Diff 286202.EditedAug 17 2020, 10:48 PM

update:

  1. move internal remarks hotness threshold type change and its related handling code from child diff into this one. With this move, remarks hotness threshold now accepts 'auto' value, but its handling is currently disabled.
  2. format change.
weiwang retitled this revision from [LTO] Make setupLLVMOptimizationRemarks accept remarks hotness threshold to [Remarks][1/2] Expand remarks hotness threshold option support in more tools.Aug 17 2020, 10:50 PM
weiwang edited the summary of this revision. (Show Details)
weiwang edited the summary of this revision. (Show Details)
weiwang marked 3 inline comments as done.
weiwang updated this revision to Diff 286466.Aug 18 2020, 8:33 PM
weiwang marked an inline comment as done.

Try to fix a windows build error. No msvc available locally, so even with this change, the build could still fail.

Suspected a naming conflict between llvm::Optional class and llvm::Optional enum defined in CommandLine.h.

weiwang updated this revision to Diff 286477.Aug 18 2020, 10:06 PM

try to fix windows build error. Use fully qualified name for enum value Optional.

weiwang updated this revision to Diff 286588.Aug 19 2020, 9:44 AM

try to fix windows build linking error.

Really appreciate the feedbacks. If there is no more, could anyone approve 3 changes? Thanks.

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

MaskRay requested changes to this revision.Sep 28 2020, 2:41 PM
MaskRay added inline comments.
lld/ELF/Config.h
115

Don't repeat the include/llvm/LTO/Config.h comment here

lld just forwards these options. Their help messages should be in llvm/LTO/

lld/ELF/Driver.cpp
1023

delete trailing period

llvm/include/llvm/Remarks/HotnessThresholdParser.h
21

empty line before namespace

llvm/include/llvm/Support/CommandLine.h
1484

Unrelated change

This revision now requires changes to proceed.Sep 28 2020, 2:41 PM
weiwang updated this revision to Diff 296593.Oct 6 2020, 9:34 PM

Address Comments.

weiwang marked 3 inline comments as done.Oct 6 2020, 10:02 PM

Thanks @MaskRay ! I am really sorry I didn't catch up your comments earlier. My email somehow stops giving me notifications on diff updates.

llvm/include/llvm/Support/CommandLine.h
1484

This change is need for window msvc build test to pass. Previously the msvc build failed during template instantiation at cl::opt<Optional<uint64_t>, false, remarks::HotnessThresholdParser>. The first argument for the base class ctor is an enum type llvm::cl::Optional, but msvc somehow tries to map it to llvm::Optional. I am not sure why the compiler thinks they are the same. I've tried several ways to fix this, but it only works by explicitly giving its fully qualified name. Linux build does not have this issue.

@MaskRay @tejohnson, do you have other comments regarding this change and its dependent?

LGTM, but please wait for @MaskRay since he had the most recent comments on this patch.

Thanks! I will wait for his input.

MaskRay added inline comments.Tue, Nov 17, 1:45 PM
lld/ELF/Options.td
553

This is not done

llvm/include/llvm/LTO/Config.h
128

Does swapping the meaning of 0 and None make more sense? None looks like a better default value.

llvm/include/llvm/Remarks/HotnessThresholdParser.h
48

Regarding the MSVC compiliation issue. If you use llvm::Optional here, does it work?

llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll
12

Full stop.

30

We don't use not FileCheck. If you want to check a pattern does not exist, use https://llvm.org/docs/CommandGuide/FileCheck.html#the-check-not-directive

53

Please fix the not FIleCheck

weiwang added inline comments.Tue, Nov 17, 2:35 PM
lld/ELF/Options.td
553

Since the value can be integer or 'auto', maybe <value> is more appropriate here?

weiwang added inline comments.Tue, Nov 17, 3:31 PM
llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll
30

Sorry, I am not sure how to apply "CHECK-EMPTY" in this case. The input is empty, and I think "CHECK-EMPTY" assumes a non-empty input (requires a "CHECK" line above it).

weiwang added inline comments.Tue, Nov 17, 3:38 PM
llvm/include/llvm/Remarks/HotnessThresholdParser.h
48

Good point! I completed overlooked this place. I will give it a try in the next revision.

MaskRay added inline comments.Tue, Nov 17, 4:32 PM
llvm/test/LTO/Resolution/X86/diagnostic-handler-remarks-with-hotness.ll
30

Use | count 0 to check empty output

weiwang added inline comments.Tue, Nov 17, 5:08 PM
llvm/include/llvm/LTO/Config.h
128

None means unknown, more of a transient value. It would be updated by the actual hotness value from PSI later. Also currently 0 is treated as disabled, I figured keeping its current meaning requires less change.

weiwang updated this revision to Diff 305929.Tue, Nov 17, 5:08 PM

update test case

weiwang marked 4 inline comments as done.Tue, Nov 17, 5:10 PM
weiwang updated this revision to Diff 305934.Tue, Nov 17, 5:25 PM

update to address MSVC build error

weiwang added inline comments.Tue, Nov 17, 5:39 PM
llvm/include/llvm/Remarks/HotnessThresholdParser.h
48

Well, just tried it, looks like the windows build failed at the same place:
https://buildkite.com/llvm-project/premerge-checks/builds/17124

FAILED: lib/LTO/CMakeFiles/LLVMLTO.dir/LTOCodeGenerator.cpp.obj 
sccache C:\BuildTools\VC\Tools\MSVC\14.27.29110\bin\Hostx64\x64\cl.exe  /nologo /TP -DBUILD_EXAMPLES -DGTEST_HAS_RTTI=0 -DUNICODE -D_CRT_NONSTDC_NO_DEPRECATE -D_CRT_NONSTDC_NO_WARNINGS -D_CRT_SECURE_NO_DEPRECATE -D_CRT_SECURE_NO_WARNINGS -D_HAS_EXCEPTIONS=0 -D_SCL_SECURE_NO_DEPRECATE -D_SCL_SECURE_NO_WARNINGS -D_UNICODE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib\LTO -IC:\ws\w64\llvm-project\premerge-checks\llvm\lib\LTO -Iinclude -IC:\ws\w64\llvm-project\premerge-checks\llvm\include /DWIN32 /D_WINDOWS   /Zc:inline /Zc:__cplusplus /Zc:strictStrings /Oi /Zc:rvalueCast /bigobj /W4 -wd4141 -wd4146 -wd4244 -wd4267 -wd4291 -wd4351 -wd4456 -wd4457 -wd4458 -wd4459 -wd4503 -wd4624 -wd4722 -wd4100 -wd4127 -wd4512 -wd4505 -wd4610 -wd4510 -wd4702 -wd4245 -wd4706 -wd4310 -wd4701 -wd4703 -wd4389 -wd4611 -wd4805 -wd4204 -wd4577 -wd4091 -wd4592 -wd4319 -wd4709 -wd4324 -w14062 -we4238 /Gw /MD /O2 /Ob2    /EHs-c- /GR- -UNDEBUG -std:c++14 /showIncludes /Folib\LTO\CMakeFiles\LLVMLTO.dir\LTOCodeGenerator.cpp.obj /Fdlib\LTO\CMakeFiles\LLVMLTO.dir\LLVMLTO.pdb /FS -c C:\ws\w64\llvm-project\premerge-checks\llvm\lib\LTO\LTOCodeGenerator.cpp
C:\ws\w64\llvm-project\premerge-checks\llvm\include\llvm/Support/CommandLine.h(1484): error C2275: 'llvm::Optional<uint64_t>': illegal use of this type as an expression
C:\ws\w64\llvm-project\premerge-checks\llvm\lib\LTO\LTOCodeGenerator.cpp(97): note: see reference to function template instantiation 'llvm::cl::opt<llvm::Optional<uint64_t>,false,llvm::remarks::HotnessThresholdParser>::opt<char[35],llvm::cl::desc,llvm::cl::value_desc,llvm::cl::initializer<int>,llvm::cl::OptionHidden>(const char (&)[35],const llvm::cl::desc &,const llvm::cl::value_desc &,const llvm::cl::initializer<int> &,const llvm::cl::OptionHidden &)' being compiled
C:\ws\w64\llvm-project\premerge-checks\llvm\lib\LTO\LTOCodeGenerator.cpp(92): note: see reference to function template instantiation 'llvm::cl::opt<llvm::Optional<uint64_t>,false,llvm::remarks::HotnessThresholdParser>::opt<char[35],llvm::cl::desc,llvm::cl::value_desc,llvm::cl::initializer<int>,llvm::cl::OptionHidden>(const char (&)[35],const llvm::cl::desc &,const llvm::cl::value_desc &,const llvm::cl::initializer<int> &,const llvm::cl::OptionHidden &)' being compiled

I am going to revert it back.

weiwang updated this revision to Diff 305942.Tue, Nov 17, 5:50 PM

revert back MSVC fix

@tejohnson @MaskRay Do you have other comments?

MaskRay accepted this revision.Mon, Nov 30, 11:30 AM
This revision is now accepted and ready to land.Mon, Nov 30, 8:20 PM
This revision was landed with ongoing or failed builds.Mon, Nov 30, 9:57 PM
This revision was automatically updated to reflect the committed changes.