This is an archive of the discontinued LLVM Phabricator instance.

[clang] Pass-through remarks options to linker
ClosedPublic

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

Details

Summary

Propagate driver commandline remarks options to linker when LTO is enabled.

This gives novice user a convenient way to collect and filter remarks throughout
a typical toolchain invocation with sample profile and LTO using single switch
from the clang driver.

A typical use of this option from clang command-line:

  • Using -Rpass* options to print remarks to screen:
clang -fuse-ld=lld -flto=thin -fprofile-sample-use=foo_sample.txt
  -Rpass=inline -Rpass-missed=inline -Rpass-analysis=inline
  -fdiagnostics-show-hotness -fdiagnostics-hotness-threshold=100 -o foo foo.cpp

Remarks will be dumped to screen from both pre-lto and lto compilation.

  • Using serialized remarks options
clang -fuse-ld=lld -flto=thin -fprofile-sample-use=foo_sample.txt
  -fsave-optimization-record
  -fdiagnostics-show-hotness -fdiagnostics-hotness-threshold=100 -o foo foo.cpp

This will produce multiple yaml files containing optimization remarks:

  1. foo.opt.yaml : remarks from pre-lto
  2. foo.opt.ld.yaml.thin.1.yaml: remark during lto

Diff Detail

Event Timeline

weiwang created this revision.Aug 11 2020, 10:44 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 11 2020, 10:44 PM
weiwang requested review of this revision.Aug 11 2020, 10:44 PM
weiwang added reviewers: wenlei, hoyFB.

This is the 2nd 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 updated this revision to Diff 285205.Aug 12 2020, 4:18 PM

update test case

MaskRay added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
71

Checking the path is brittle. Consider Args.getLastArgValue(options::OPT_fuse_ld_EQ) == "lld"

73

return isLLD;

The Mach-O port is a WIP. You don't need to handle Triple.isOSDarwin() && Args.getAllArgValues(options::OPT_arch).size() > 1; currently.

This suggests that this simple function should be simply inlined.

81

Delete such one-shot variables

Use Twine

126

(F + Twine(".opt.ld.") + Format).str()

597
602

Full stop after a complete sentence.

Capitalize

603
clang/test/Driver/remarks-pass-through.c
1 ↗(On Diff #285205)

Consider reusing opt-record.c

The current filename makes it difficult to find related tests of -fsave-optimization-record

24 ↗(On Diff #285205)

Replace // with an empty line

MaskRay requested changes to this revision.Aug 13 2020, 8:38 PM
This revision now requires changes to proceed.Aug 13 2020, 8:38 PM
tejohnson added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
71

Would be better to add equivalent support to gold-plugin.cpp, which uses the same lto::Config, and then you don't need the lld check. Just use --plugin-opt instead of -mllvm for passing down the internal options, that is recognized by lld as well (see other examples here).

weiwang updated this revision to Diff 286124.Aug 17 2020, 1:12 PM

update:

  1. add --plugin-opt alias for remarks in lld
  2. handle both lld and ld.gold pass-through
  3. simplify target checking logic
  4. code format update
weiwang marked 8 inline comments as done.Aug 17 2020, 1:14 PM
weiwang retitled this revision from [clang] Pass-through remarks options to lld to [clang] Pass-through remarks options to linker.
weiwang edited the summary of this revision. (Show Details)
MaskRay added inline comments.Aug 17 2020, 2:16 PM
lld/ELF/Options.td
595 ↗(On Diff #286124)

The change should be moved to a previous patch which will add --opt-remarks-* options.

weiwang updated this revision to Diff 286204.Aug 17 2020, 10:56 PM

move some unrelated change into parent diff.

Hi Wei, this looks handy! Minor stylish comments below.

clang/include/clang/Driver/Driver.h
638

Nitpicking here, how about usesRpassOptions?

clang/lib/Driver/ToolChains/CommonArgs.cpp
67

You can also remove curly braces from here.

72

and here.

77

and here.

99

and here, hoist the comment above the if.

weiwang updated this revision to Diff 286467.Aug 18 2020, 8:35 PM

coding style change

weiwang marked 5 inline comments as done.Aug 18 2020, 8:36 PM
weiwang added inline comments.
clang/include/clang/Driver/Driver.h
638

Thanks! Suggested name makes more sense. fixed.

weiwang updated this revision to Diff 286468.Aug 18 2020, 8:38 PM
weiwang marked an inline comment as done.

fix typo

Looks pretty good but I think it can be simplified in one place as I note below.

clang/lib/Driver/ToolChains/CommonArgs.cpp
596

Is this guard needed? It looks like renderRpassOptions will do the right thing if none are specified (won't add anything). Right now you end up checking each one a second time if any are enabled.

weiwang added inline comments.Aug 27 2020, 1:27 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
596

You are right! I will remove the redundant check.

weiwang updated this revision to Diff 288447.Aug 27 2020, 1:43 PM

remove redundant check

thegameg accepted this revision.Sep 17 2020, 2:58 PM
thegameg added a subscriber: thegameg.

LGTM

MaskRay added inline comments.Sep 17 2020, 3:20 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
90

The output selection logic is untested.

clang/test/Driver/opt-record.c
24

x86_64-linux-gnu can usually be simplied as x86_64.

There is no test without -o FOO.

Consider moving the RUN lines below, immediately above CHECK-PASS: lines

56

Add -SAME whenever appropriate.

MaskRay requested changes to this revision.Sep 17 2020, 3:21 PM
This revision now requires changes to proceed.Sep 17 2020, 3:21 PM
weiwang updated this revision to Diff 292676.Sep 17 2020, 5:20 PM

update test case

weiwang marked an inline comment as done.Sep 17 2020, 5:25 PM
weiwang added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
90

I am not sure how to trigger a case of Output not being a filename in test case.

clang/test/Driver/opt-record.c
24

x86_64 somehow gives error for the toolchain to generate linker command. I've updated the triple to be x86_64-linux.

Without -o FOO, it would just use the default output name a.out.

As suggested, changed lines are moved together.

MaskRay added inline comments.Sep 18 2020, 9:16 PM
clang/lib/Driver/ToolChains/CommonArgs.cpp
95

Is the F.empty() case unreachable? Please delete the code or add an assert

clang/test/Driver/opt-record.c
57

This looks strange. We don't test with not FileCheck

You are right that we should specify x86_64-linux because otherwise the command line is constructed from gcc (rarely used bare metal linking).

Suggest: use this RUN line to test the case without -o

weiwang updated this revision to Diff 293276.Sep 21 2020, 4:07 PM
  1. remove unreachable code
  2. udpate test case
weiwang marked 2 inline comments as done.Sep 21 2020, 4:08 PM
MaskRay accepted this revision.Sep 21 2020, 4:34 PM
MaskRay added inline comments.
clang/lib/Driver/ToolChains/CommonArgs.cpp
104

Delete .data()

clang/test/Driver/opt-record.c
65

I'd write // CHECK-NOPASS-NOT: "--plugin-opt=opt-remarks-filename=

This revision is now accepted and ready to land.Sep 21 2020, 4:34 PM
weiwang updated this revision to Diff 293296.Sep 21 2020, 4:58 PM

minor update to test case

weiwang marked an inline comment as done.Sep 21 2020, 4:59 PM
weiwang edited the summary of this revision. (Show Details)Oct 27 2020, 5:22 PM
weiwang closed this revision.Oct 27 2020, 5:33 PM

Diff was committed, but did not close automatically. Manual close it now.