[clang] Add command line options for the Machine Function Splitter.

[snehasish]

Summary

This patch adds a command line flag for the machine function splitter
(added in rG94faadaca4e1). The command line reference has been updated
with the new flag and tests added to check the pass is run (requires
profile data) and that the driver has registered the correct option. A
warning is emitted if the option is enabled when no profile is provided.

-fsplit-machine-functions
Split machine functions using profile information (x86-elf only). On
other targets an error is emitted. If profile information is not
provided a warning is emitted notifying the user of backend
optimizations failure.

[davidxl]

For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified?

Also is it worth to give a warning if the option is specified but PGO is not on?

[snehasish]

Add warning when option is enabled without profile.

[snehasish]

Thanks for the comments! PTAL.

In D87047#2252982, @davidxl wrote:

For x86 target, should it be turned on when -fprofile-use= option is specified unless -fno-split-machine-function is specified?

I don't think we are there yet to turn it on for x86 profile builds. There is at least one unresolved issue with DebugInfo (D85085); once we have tested this some more internally we can revisit and make it the default.

Also is it worth to give a warning if the option is specified but PGO is not on?

Added a warning if no profile is specified but this option is turned on.

[tmsriram]

This LGTM.

[snehasish]

Add a check for x86-elf.

• Add a check for the triple in the driver to only enable the flag on x86+elf targets.
• Add a test to ensure we don't enable this on other targets.

[snehasish]

Updated test and warning type.

• Updated test to check for the diagnostic message.
• Updated BackendUtil.cpp to use backend optimization failure warning.

[snehasish]

Check warning, specify target to avoid failures on windows.

• Added a check for warning emitted if no profile is provided.
• Added a triple for the remaining checks since we now emit an error for incompatible targets.

[MaskRay]

clang/include/clang/Driver/Options.td

1999

Please use OptInFFlag and see its comment.

clang/lib/Driver/ToolChains/Clang.cpp

4259

This is not needed.

This is for fembed-bitcode and people seem to randomly add options here. Many options are probably not needed.

4920

If you use getLastArg

A->getAsString(Args)

clang/test/CodeGen/split-machine-functions.c

3 ↗

(On Diff #291063)

Consider RUN: split-file

Search for this string for some examples.

[snehasish]

Use OptInFFlag, split-file and update tests.

• Change the flag type to OptInFFlag.
• Use split-file in the test to avoid "RUN: echo" lines.
• Use an existing warn message (if no profile is available) and add a check for it.

[snehasish]

PTAL, thanks!

clang/lib/Driver/ToolChains/Clang.cpp

4259

Thanks for catching this. I read this list as "options which should be ignored for embedding bitcode". In this case we do want to save this flag to pass back to clang if necessary (as mtrofin@ pointed out offline). Perhaps the documentation around this could be enhanced.

clang/test/CodeGen/split-machine-functions.c

3 ↗

(On Diff #291063)

Looks much cleaner with split-file overall, though it doesn't play well with clang-format since this is a .c file. Should we just ignore clang-format complaints for this file?

[snehasish]

Fix test formatting.

Added clang-format off to disable clang format for the test which contains both profile data (as text) and c code.

[snehasish]

clang/test/CodeGen/split-machine-functions.c

3 ↗

(On Diff #291063)

Fixed the formatting by adding a clang-format off directive at the top of this file.

[MaskRay]

clang/docs/ClangCommandLineReference.rst

2131 ↗

(On Diff #291122)

You don't need to edit this file. It is auto-generated from Options.td

clang/lib/Driver/ToolChains/Clang.cpp

4917

OPT_fno_split_machine_functions is a driver option but not a CC1 option. Rendering OPT_fno_split_machine_functions will error when processing CC1 options.

clang/test/CodeGen/split-machine-functions.c

1 ↗

(On Diff #291122)

You can ignore clang-format/clang-tidy issues in test/ and there is no need to annotate the tests (otherwise we will annotate essentially every file)

5 ↗

(On Diff #291122)

See https://reviews.llvm.org/D85408#2266559

It feels wrong that the assembly+llvm-profdata test is in clang/test

clang/test/Driver/fsplit-machine-functions.c

2

Add -fprofile-use to the canonical usage line

4

Two -c?

[snehasish]

Remove clang/CodeGen test, update arg render logic.

• Removed the clang/CodeGen test.
• Added a check for the option to be rendered.
• Fixed extra flags in driver test.

[snehasish]

It feels wrong that the assembly+llvm-profdata test is in clang/test

I agree with @dblaikie and your assessment that it feels wrong to add such a test to clang. In the first version of this patch, the test served the purpose of representing the canonical usage with profdata as well as check for the plumbing of the profile along with the presence of the flag. Enhancing the driver test removed the need for the former while adding a diagnostic warning message (and a check for it) removes the need for the latter. I've removed the clang codegen test.

Thanks for the detailed review, PTAL.

[snehasish]

Rebased patch.

[snehasish]

@MaskRay ping, let me know if you have any further comments. Thanks!

[MaskRay]

clang/test/Driver/fsplit-machine-functions.c

4

There are two -c on this line.

Testing warnings can use -### as well.

[snehasish]

Remove extra -c from test command line.

[snehasish]

clang/test/Driver/fsplit-machine-functions.c

4

Removed the extra -c.
The warning is emitted when CreateTargetMachine (and its children) are called. So using -### instead of -c for the CHECK-WARN test is not sufficient as far as I can tell.

[MaskRay]

clang/test/Driver/fsplit-machine-functions.c

4

If you make a compile action, this may need x86-registered-target.

[MaskRay]

clang/lib/CodeGen/BackendUtil.cpp

517

I did not pay attention. Such compatibility checks should be added in lib/Driver, instead of lib/CodeGen.

clang/test/Driver/fsplit-machine-functions.c

4

I did not pay attention. If you added checks to lib/Driver, x86-registered-target would not be needed and you could use -###

[snehasish]

Check profile flag in Driver, update test.

• Move the profile missing warning check to the Driver.
• Update the test to use -### for CHECK-WARN.

[snehasish]

That makes sense. I moved the check to lib/Driver/ToolChains/Clang.cpp and updated the test. Seems cleaner to have all the checks in one place.
PTAL, thanks.

[MaskRay]

clang/include/clang/Driver/Options.td

2000

If this can be adapted to other targets, there is no need to specifically mention x86-elf only ("x86 ELF")

clang/lib/CodeGen/BackendUtil.cpp

12

This is not needed

clang/lib/Driver/ToolChains/Clang.cpp

27

Not needed?

[snehasish]

Remove unnecessary includes, update doc text.

• Update doctext.
• Remove no longer necessary includes to frontend driver diagnostics.

[MaskRay]

clang/include/clang/Driver/Options.td

2000

So I take quickly a look at rG94faadaca4e1704f674d2e9d4a1d25643b9ca52c. There is nothing ELF specific, right? At least I don't expect AArch64 ELF to fail. If you do think this merits a "ELF only" comment, adding it seems feasible.

clang/test/Driver/fsplit-machine-functions.c

9

For diagnostics, we usually add the warning or error prefix.

[snehasish]

Update the test.

• Added "warning: " prefix to be checked.

[snehasish]

Thanks for the review.

clang/include/clang/Driver/Options.td

2000

The machine function splitter uses the basic block sections feature. This has not been tested on AArch64, there is some target specific code in the CFI handling as well idiosyncrasies of each target which need to be worked out. I'm going to leave this as it is since the checks included in this patch enforce x86 & ELF. This is on our longer term roadmap and we will revisit this in the future.

clang/include/clang/Basic/CodeGenOptions.def

 CODEGENOPT(EnableSegmentedStacks , 1, 0) ///< Set when -fsplit-stack is enabled.
 CODEGENOPT(NoInlineLineTables, 1, 0) ///< Whether debug info should contain
                                      ///< inline line tables.
 CODEGENOPT(StackClashProtector, 1, 0) ///< Set when -fstack-clash-protection is enabled.
 CODEGENOPT(NoImplicitFloat   , 1, 0) ///< Set when -mno-implicit-float is enabled.
 CODEGENOPT(NullPointerIsValid , 1, 0) ///< Assume Null pointer deference is defined.
 CODEGENOPT(CorrectlyRoundedDivSqrt, 1, 0) ///< -cl-fp32-correctly-rounded-divide-sqrt
 CODEGENOPT(UniqueInternalLinkageNames, 1, 0) ///< Internal Linkage symbols get unique names.
+CODEGENOPT(SplitMachineFunctions, 1, 0) ///< Split machine functions using profile information.
 
 /// When false, this attempts to generate code as if the result of an
 /// overflowing conversion matches the overflowing behavior of a target's native
 /// float-to-int conversion instructions.
 CODEGENOPT(StrictFloatCastOverflow, 1, 1)
 
 CODEGENOPT(UniformWGSize     , 1, 0) ///< -cl-uniform-work-group-size
 CODEGENOPT(NoZeroInitializedInBSS , 1, 0) ///< -fno-zero-initialized-in-bss.

clang/include/clang/Driver/Options.td

 
 defm unique_basic_block_section_names : OptInFFlag<"unique-basic-block-section-names",
  "Use unique names for basic block sections (ELF Only)">;
 defm unique_internal_linkage_names : OptInFFlag<"unique-internal-linkage-names",
  "Uniqueify Internal Linkage Symbol Names by appending the MD5 hash of the module path">;
 defm unique_section_names : OptOutFFlag<"unique-section-names",
   "", "Don't use unique names for text and data sections">;
 
+defm split_machine_functions: OptInFFlag<"split-machine-functions",
+  "Enable", "Disable", " late function splitting using profile information (x86 ELF)">;
+
 defm strict_return : OptOutFFlag<"strict-return", "",
   "Don't treat control flow paths that fall off the end of a non-void function as unreachable">;
 
 def fenable_matrix : Flag<["-"], "fenable-matrix">, Group<f_Group>,
     Flags<[CC1Option]>,
     HelpText<"Enable matrix data type and related builtin functions">;
 
 

clang/lib/CodeGen/BackendUtil.cpp

 //===--- BackendUtil.cpp - LLVM Backend Utilities -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "clang/CodeGen/BackendUtil.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/Diagnostic.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/TargetOptions.h"
 #include "clang/Frontend/FrontendDiagnostic.h"
 #include "clang/Frontend/Utils.h"
 #include "clang/Lex/HeaderSearchOptions.h"
 #include "llvm/ADT/SmallSet.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/ADT/StringSwitch.h"
 #include "llvm/ADT/Triple.h"
ErrorOr<std::unique_ptr<MemoryBuffer>> MBOrErr =
         MemoryBuffer::getFile(CodeGenOpts.BBSections.substr(5));
     if (!MBOrErr)
       Diags.Report(diag::err_fe_unable_to_load_basic_block_sections_file)
           << MBOrErr.getError().message();
     else
       Options.BBSectionsFuncListBuf = std::move(*MBOrErr);
   }
 
+  Options.EnableMachineFunctionSplitter = CodeGenOpts.SplitMachineFunctions;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
   Options.UniqueBasicBlockSectionNames =
       CodeGenOpts.UniqueBasicBlockSectionNames;
   Options.TLSSize = CodeGenOpts.TLSSize;
   Options.EmulatedTLS = CodeGenOpts.EmulatedTLS;
   Options.ExplicitEmulatedTLS = CodeGenOpts.ExplicitEmulatedTLS;

clang/lib/Driver/ToolChains/Clang.cpp

 #include "Arch/X86.h"
 #include "CommonArgs.h"
 #include "Hexagon.h"
 #include "InputInfo.h"
 #include "MSP430.h"
 #include "PS4CPU.h"
 #include "clang/Basic/CharInfo.h"
 #include "clang/Basic/CodeGenOptions.h"
 #include "clang/Basic/LangOptions.h"
 #include "clang/Basic/ObjCRuntime.h"
 #include "clang/Basic/Version.h"
 #include "clang/Driver/Distro.h"
 #include "clang/Driver/DriverDiagnostic.h"
 #include "clang/Driver/Options.h"
 #include "clang/Driver/SanitizerArgs.h"
 #include "clang/Driver/XRayArgs.h"
 #include "llvm/ADT/StringExtras.h"
static const constexpr unsigned kBitcodeOptionBlacklist[] = {
         options::OPT_fno_data_sections,
         options::OPT_fbasic_block_sections_EQ,
         options::OPT_funique_internal_linkage_names,
         options::OPT_fno_unique_internal_linkage_names,
         options::OPT_funique_section_names,
         options::OPT_fno_unique_section_names,
         options::OPT_funique_basic_block_section_names,
         options::OPT_fno_unique_basic_block_section_names,
         options::OPT_mrestrict_it,
         options::OPT_mno_restrict_it,
         options::OPT_mstackrealign,
         options::OPT_mno_stackrealign,
         options::OPT_mstack_alignment,
         options::OPT_mcmodel_EQ,
         options::OPT_mlong_calls,
         options::OPT_mno_long_calls,
         options::OPT_ggnu_pubnames,
#endif
   if (Args.hasFlag(options::OPT_funique_internal_linkage_names,
                    options::OPT_fno_unique_internal_linkage_names, false))
     CmdArgs.push_back("-funique-internal-linkage-names");
 
   if (Args.hasFlag(options::OPT_funique_basic_block_section_names,
                    options::OPT_fno_unique_basic_block_section_names, false))
     CmdArgs.push_back("-funique-basic-block-section-names");
 
+  if (Arg *A = Args.getLastArg(options::OPT_fsplit_machine_functions,
+                               options::OPT_fno_split_machine_functions)) {
+    // This codegen pass is only available on x86-elf targets.
+    if (Triple.isX86() && Triple.isOSBinFormatELF()) {
+      if (A->getOption().matches(options::OPT_fsplit_machine_functions)) {
+        // If the flag is enabled but no profile information is available then
+        // emit a warning.
+        if (getLastProfileUseArg(Args) || getLastProfileSampleUseArg(Args)) {
+          A->render(Args, CmdArgs);
+        } else {
+          D.Diag(diag::warn_drv_diagnostics_hotness_requires_pgo)
+              << A->getAsString(Args);
+        }
+      }
+    } else {
+      D.Diag(diag::err_drv_unsupported_opt_for_target)
+          << A->getAsString(Args) << TripleStr;
+    }
+  }
+
   Args.AddLastArg(CmdArgs, options::OPT_finstrument_functions,
                   options::OPT_finstrument_functions_after_inlining,
                   options::OPT_finstrument_function_entry_bare);
 
   // NVPTX/AMDGCN doesn't support PGO or coverage. There's no runtime support
   // for sampling, overhead of call arc collection is way too high and there's
   // no way to collect the output.
   if (!Triple.isNVPTX() && !Triple.isAMDGCN())

clang/lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.DataSections = Args.hasArg(OPT_fdata_sections);
   Opts.StackSizeSection = Args.hasArg(OPT_fstack_size_section);
   Opts.UniqueSectionNames = !Args.hasArg(OPT_fno_unique_section_names);
   Opts.UniqueBasicBlockSectionNames =
       Args.hasArg(OPT_funique_basic_block_section_names);
   Opts.UniqueInternalLinkageNames =
       Args.hasArg(OPT_funique_internal_linkage_names);
 
+  Opts.SplitMachineFunctions = Args.hasArg(OPT_fsplit_machine_functions);
+
   Opts.MergeFunctions = Args.hasArg(OPT_fmerge_functions);
 
   Opts.NoUseJumpTables = Args.hasArg(OPT_fno_jump_tables);
 
   Opts.NullPointerIsValid = Args.hasArg(OPT_fno_delete_null_pointer_checks);
 
   Opts.ProfileSampleAccurate = Args.hasArg(OPT_fprofile_sample_accurate);
 

clang/test/Driver/fsplit-machine-functions.c

+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -target x86_64 -fprofile-use=default.profdata -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target x86_64 -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-WARN %s
+// RUN: not %clang -c -target arm-unknown-linux -fsplit-machine-functions %s 2>&1 | FileCheck -check-prefix=CHECK-TRIPLE %s
+
+// CHECK-OPT:       "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"
+// CHECK-WARN:      warning: argument '-fsplit-machine-functions' requires profile-guided optimization information
+// CHECK-TRIPLE:    error: unsupported option '-fsplit-machine-functions' for target