[clang] Pass-through remarks options to linker

[weiwang]

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

[weiwang]

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]

update test case

[MaskRay]

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()

614

http://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

619

Full stop after a complete sentence.

Capitalize

620

http://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

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

[tejohnson]

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]

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

[MaskRay]

lld/ELF/Options.td

595 ↗

(On Diff #286124)

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

[weiwang]

move some unrelated change into parent diff.

[bruno]

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

clang/include/clang/Driver/Driver.h

638 ↗

(On Diff #286204)

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]

coding style change

[weiwang]

clang/include/clang/Driver/Driver.h

638 ↗

(On Diff #286204)

Thanks! Suggested name makes more sense. fixed.

[weiwang]

fix typo

[tejohnson]

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

clang/lib/Driver/ToolChains/CommonArgs.cpp

613

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]

clang/lib/Driver/ToolChains/CommonArgs.cpp

613

You are right! I will remove the redundant check.

[weiwang]

remove redundant check

[thegameg]

LGTM

[MaskRay]

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

46

Add -SAME whenever appropriate.

[weiwang]

update test case

[weiwang]

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]

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

47

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]

1. remove unreachable code
2. udpate test case

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

104

Delete .data()

clang/test/Driver/opt-record.c

55

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

[weiwang]

minor update to test case

[weiwang]

fix typo

[weiwang]

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

clang/lib/Driver/ToolChains/CommonArgs.cpp

 #include "llvm/Support/VirtualFileSystem.h"
 #include "llvm/Support/YAMLParser.h"
 
 using namespace clang::driver;
 using namespace clang::driver::tools;
 using namespace clang;
 using namespace llvm::opt;
 
+static void renderRpassOptions(const ArgList &Args, ArgStringList &CmdArgs) {
+  if (const Arg *A = Args.getLastArg(options::OPT_Rpass_EQ))
+    CmdArgs.push_back(Args.MakeArgString(Twine("--plugin-opt=-pass-remarks=") +
+                                         A->getValue()));
+
+  if (const Arg *A = Args.getLastArg(options::OPT_Rpass_missed_EQ))
+    CmdArgs.push_back(Args.MakeArgString(
+        Twine("--plugin-opt=-pass-remarks-missed=") + A->getValue()));
+
+  if (const Arg *A = Args.getLastArg(options::OPT_Rpass_analysis_EQ))
+    CmdArgs.push_back(Args.MakeArgString(
+        Twine("--plugin-opt=-pass-remarks-analysis=") + A->getValue()));
+}
+
+static void renderRemarksOptions(const ArgList &Args, ArgStringList &CmdArgs,
+                                 const llvm::Triple &Triple,
+                                 const InputInfo &Input,
+                                 const InputInfo &Output) {
+  StringRef Format = "yaml";
+  if (const Arg *A = Args.getLastArg(options::OPT_fsave_optimization_record_EQ))
+    Format = A->getValue();
+
+  SmallString<128> F;
+  const Arg *A = Args.getLastArg(options::OPT_foptimization_record_file_EQ);
+  if (A)
+    F = A->getValue();
+  else if (Output.isFilename())
+    F = Output.getFilename();
+
+  assert(!F.empty() && "Cannot determine remarks output name.");
+  // Append "opt.ld.<format>" to the end of the file name.
+  CmdArgs.push_back(
+      Args.MakeArgString(Twine("--plugin-opt=opt-remarks-filename=") + F +
+                         Twine(".opt.ld.") + Format));
+
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_foptimization_record_passes_EQ))
+    CmdArgs.push_back(Args.MakeArgString(
+        Twine("--plugin-opt=opt-remarks-passes=") + A->getValue()));
+
+  CmdArgs.push_back(Args.MakeArgString(
+      Twine("--plugin-opt=opt-remarks-format=") + Format.data()));
+}
+
+static void renderRemarksHotnessOptions(const ArgList &Args,
+                                        ArgStringList &CmdArgs) {
+  if (Args.hasFlag(options::OPT_fdiagnostics_show_hotness,
+                   options::OPT_fno_diagnostics_show_hotness, false))
+    CmdArgs.push_back("--plugin-opt=opt-remarks-with-hotness");
+
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_fdiagnostics_hotness_threshold_EQ))
+    CmdArgs.push_back(Args.MakeArgString(
+        Twine("--plugin-opt=opt-remarks-hotness-threshold=") + A->getValue()));
+}
+
 void tools::addPathIfExists(const Driver &D, const Twine &Path,
                             ToolChain::path_list &Paths) {
   if (D.getVFS().exists(Path))
     Paths.push_back(Path.str());
 }
 
 void tools::handleTargetFeaturesGroup(const ArgList &Args,
                                       std::vector<StringRef> &Features,
                                       OptSpecifier Group) {
   for (const Arg *A : Args.filtered(Group)) {
     StringRef Name = A->getOption().getName();
     A->claim();
 
     // Skip over "-m".
     assert(Name.startswith("m") && "Invalid feature name.");
     Name = Name.substr(1);
#endif
 
   // Setup statistics file output.
   SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
   if (!StatsFile.empty())
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-plugin-opt=stats-file=") + StatsFile));
 
   addX86AlignBranchArgs(D, Args, CmdArgs, /*IsLTO=*/true);
+
+  // Handle remark diagnostics on screen options: '-Rpass-*'.
+  renderRpassOptions(Args, CmdArgs);
+
+  // Handle serialized remarks options: '-fsave-optimization-record'
+  // and '-foptimization-record-*'.
+  if (willEmitRemarks(Args))
+    renderRemarksOptions(Args, CmdArgs, ToolChain.getEffectiveTriple(), Input,
+                         Output);
+
+  // Handle remarks hotness/threshold related options.
+  renderRemarksHotnessOptions(Args, CmdArgs);
 }
 
 void tools::addArchSpecificRPath(const ToolChain &TC, const ArgList &Args,
                                  ArgStringList &CmdArgs) {
   // Enable -frtlib-add-rpath by default for the case of VE.
   const bool IsVE = TC.getTriple().isVE();
   bool DefaultValue = IsVE;
   if (!Args.hasFlag(options::OPT_frtlib_add_rpath,

clang/test/Driver/opt-record.c

 // RUN: %clang -### -S -o FOO -foptimization-record-passes=inline %s 2>&1 | FileCheck %s -check-prefix=CHECK-EQ-PASSES
 // RUN: %clang -### -S -o FOO -foptimization-record-passes=inline -fno-save-optimization-record %s 2>&1 | FileCheck %s --check-prefix=CHECK-FOPT-DISABLE-PASSES
 // RUN: %clang -### -S -o FOO -fsave-optimization-record -fsave-optimization-record=some-format %s 2>&1 | FileCheck %s -check-prefix=CHECK-EQ-FORMAT
 // RUN: %clang -### -S -o FOO -fsave-optimization-record=some-format %s 2>&1 | FileCheck %s -check-prefix=CHECK-EQ-FORMAT
 // RUN: %clang -### -S -o FOO -fsave-optimization-record=some-format -fno-save-optimization-record %s 2>&1 | FileCheck %s --check-prefix=CHECK-FOPT-DISABLE-FORMAT
 //
 // CHECK: "-cc1"
 // CHECK: "-opt-record-file" "FOO.opt.yaml"
 
 // CHECK-NO-O: "-cc1"
 // CHECK-NO-O-DAG: "-opt-record-file" "opt-record.opt.yaml"
 // CHECK-CUDA-DEV-DAG: "-opt-record-file" "opt-record-cuda-{{nvptx64|nvptx}}-nvidia-cuda-sm_20.opt.yaml"
 
 // CHECK-EQ: "-cc1"
 // CHECK-EQ: "-opt-record-file" "BAR.txt"
 
 // CHECK-FOPT-DISABLE-NOT: "-fno-save-optimization-record"
 
 // CHECK-EQ-PASSES: "-cc1"
 // CHECK-EQ-PASSES: "-opt-record-passes" "inline"
 
 // CHECK-FOPT-DISABLE-PASSES-NOT: "-fno-save-optimization-record"
 
 // CHECK-EQ-FORMAT: "-cc1"
 // CHECK-EQ-FORMAT: "-opt-record-file" "FOO.opt.some-format"
 // CHECK-EQ-FORMAT: "-opt-record-format" "some-format"
 
 // CHECK-FOPT-DISABLE-FORMAT-NOT: "-fno-save-optimization-record"
+
+// Test remarks options pass-through
+// No pass-through: lto is disabled
+// RUN: %clang -target x86_64 -### -o FOO -fdiagnostics-hotness-threshold=100 -fsave-optimization-record %s 2>&1 | FileCheck %s -check-prefix=CHECK-NOPASS
+
+// Pass-through:
+// RUN: %clang -target x86_64-linux -### -fuse-ld=lld -flto -fdiagnostics-hotness-threshold=100 -fsave-optimization-record -foptimization-record-passes=inline %s 2>&1 | FileCheck %s -check-prefix=CHECK-PASS-A
+// RUN: %clang -target x86_64-linux -### -o FOO -fuse-ld=gold -flto -fdiagnostics-hotness-threshold=100 -fsave-optimization-record -foptimization-record-passes=inline %s 2>&1 | FileCheck %s -check-prefix=CHECK-PASS
+// RUN: %clang -target x86_64-linux -### -o FOO -fuse-ld=lld -flto=thin -fdiagnostics-hotness-threshold=100 -fsave-optimization-record=some-format -foptimization-record-file=FOO.txt %s 2>&1 | FileCheck %s -check-prefix=CHECK-PASS-CUSTOM
+// RUN: %clang -target x86_64-linux -### -o FOO -fuse-ld=lld -flto=thin -fdiagnostics-hotness-threshold=100 -Rpass=inline -Rpass-missed=inline -Rpass-analysis=inline %s 2>&1 | FileCheck %s -check-prefix=CHECK-PASS-RPASS
+
+// CHECK-NOPASS-NOT: "--plugin-opt=opt-remarks-filename="
+// CHECK-NOPASS-NOT: "--plugin-opt=opt-remarks-passes=inline"
+// CHECK-NOPASS-NOT: "--plugin-opt=opt-remarks-format=yaml"
+// CHECK-NOPASS-NOT: "--plugin-opt=opt-remarks-hotness-threshold=100"
+
+// CHECK-PASS-A:      "--plugin-opt=opt-remarks-filename=a.out.opt.ld.yaml"
+// CHECK-PASS-A-SAME: "--plugin-opt=opt-remarks-passes=inline"
+// CHECK-PASS-A-SAME: "--plugin-opt=opt-remarks-format=yaml"
+// CHECK-PASS-A-SAME: "--plugin-opt=opt-remarks-hotness-threshold=100"
+
+// CHECK-PASS:      "--plugin-opt=opt-remarks-filename=FOO.opt.ld.yaml"
+// CHECK-PASS-SAME: "--plugin-opt=opt-remarks-passes=inline"
+// CHECK-PASS-SAME: "--plugin-opt=opt-remarks-format=yaml"
+// CHECK-PASS-SAME: "--plugin-opt=opt-remarks-hotness-threshold=100"
+
+// CHECK-PASS-CUSTOM:      "--plugin-opt=opt-remarks-filename=FOO.txt.opt.ld.some-format"
+// CHECK-PASS-CUSTOM-SAME: "--plugin-opt=opt-remarks-format=some-format"
+// CHECK-PASS-CUSTOM-SAME: "--plugin-opt=opt-remarks-hotness-threshold=100"
+
+// CHECK-PASS-RPASS:      "--plugin-opt=-pass-remarks=inline"
+// CHECK-PASS-RPASS-SAME: "--plugin-opt=-pass-remarks-missed=inline"
+// CHECK-PASS-RPASS-SAME: "--plugin-opt=-pass-remarks-analysis=inline"
+// CHECK-PASS-RPASS-SAME: "--plugin-opt=opt-remarks-hotness-threshold=100"