[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

1992

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.

4917

If you use getLastArg

A->getAsString(Args)

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

4

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

4

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

4

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

[MaskRay]

clang/docs/ClangCommandLineReference.rst

2131

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

clang/lib/Driver/ToolChains/Clang.cpp

4914

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

2

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)

6

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

1993

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

1993

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

1993

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/docs/ClangCommandLineReference.rst

 .. option:: -fsplit-dwarf-inlining, -fno-split-dwarf-inlining
 
 Provide minimal debug info in the object/executable to facilitate online symbolication/stack traces in the absence of .dwo/.dwp files when using Split DWARF
 
 .. option:: -fsplit-lto-unit, -fno-split-lto-unit
 
 Enables splitting of the LTO unit
 
+.. option:: -fsplit-machine-functions, -fno-split-machine-functions
+
+Split machine functions using profile information (x86-elf only)
+
 .. option:: -fsplit-stack
 
 .. option:: -fstack-clash-protection
 
 Enable stack clash protection
 
 .. option:: -fstack-protector, -fno-stack-protector
 

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/Basic/DiagnosticFrontendKinds.td

def err_fe_invalid_exception_model
    : Error<"invalid exception model '%0' for target '%1'">;
 def warn_fe_concepts_ts_flag : Warning<
   "-fconcepts-ts is deprecated - use '-std=c++20' for Concepts support">,
   InGroup<Deprecated>;
 
 def err_fe_unable_to_load_basic_block_sections_file : Error<
     "unable to load basic block sections function list: '%0'">;
 
+def warn_fe_machine_function_splitter_missing_profile : Warning<
+    "ignored -fsplit-machine-functions, no profile provided via -fprofile-use">;
+
 def warn_fe_serialized_diag_merge_failure : Warning<
     "unable to merge a subprocess's serialized diagnostics">,
     InGroup<SerializedDiagnostics>;
 def warn_fe_serialized_diag_failure : Warning<
     "unable to open file %0 for serializing diagnostics (%1)">,
     InGroup<SerializedDiagnostics>;
 def warn_fe_serialized_diag_failure_during_finalisation : Warning<
     "Received warning after diagnostic serialization teardown was underway: %0">,

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">;
 
+def fsplit_machine_functions : Flag <["-"], "fsplit-machine-functions">,
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Split machine functions using profile information (x86-elf only)">;
+def fno_split_machine_functions: Flag <["-"], "fno-split-machine-functions">,
+  Group<f_Group>;
+
 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);
   }
 
+  if (CodeGenOpts.SplitMachineFunctions) {
+    if (CodeGenOpts.getProfileUse() != CodeGenOptions::ProfileNone)
+      Options.EnableMachineFunctionSplitter = true;
+    else
+      Diags.Report(diag::warn_fe_machine_function_splitter_missing_profile);
+  }
+
   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_fsplit_machine_functions,
+        options::OPT_fno_split_machine_functions,
         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,
#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 (Args.hasFlag(options::OPT_fsplit_machine_functions,
+                   options::OPT_fno_split_machine_functions, false)) {
+    if (Triple.isX86() && Triple.isOSBinFormatELF())
+      CmdArgs.push_back("-fsplit-machine-functions");
+  }
+
   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())
     addPGOAndCoverageFlags(TC, C, D, Output, Args, CmdArgs);

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/CodeGen/split-machine-functions.c

+// REQUIRES: x86-registered-target
+
+// RUN: echo "foo"                > %t.proftext
+// RUN: echo "# Func Hash:"       >> %t.proftext
+// RUN: echo "11262309905"        >> %t.proftext
+// RUN: echo "# Num Counters:"    >> %t.proftext
+// RUN: echo "2"                  >> %t.proftext
+// RUN: echo "# Counter Values:"  >> %t.proftext
+// RUN: echo "1000000"            >> %t.proftext
+// RUN: echo "0"                  >> %t.proftext
+// RUN: llvm-profdata merge -o %t.profdata %t.proftext
+// RUN: %clang_cc1 -triple x86_64 -O3 -S -fprofile-instrument-use-path=%t.profdata -fsplit-machine-functions -o - < %s | FileCheck %s
+
+__attribute__((noinline)) int foo(int argc) {
+  if (argc % 2 == 0) {
+    exit(argc);
+  } else {
+    return argc + 1;
+  }
+}
+
+int main(int argc, char *argv[]) {
+  int total = 0;
+  for (int i = 0; i < 1000000; ++i) {
+    total += foo(argc);
+  }
+  printf("%d\n", total);
+}
+
+// CHECK: .section .text.hot.,"ax",@progbits
+// CHECK: foo:
+// CHECK: section .text.unlikely.foo,"ax",@progbits
+// CHECK: foo.cold:
+// CHECK: callq exit@PLT

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

+// RUN: %clang -### -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-OPT %s
+// RUN: %clang -### -fsplit-machine-functions -fno-split-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// RUN: %clang -### -target arm-unknown-linux -fsplit-machine-functions %s -c 2>&1 | FileCheck -check-prefix=CHECK-NOOPT %s
+// CHECK-OPT: "-fsplit-machine-functions"
+// CHECK-NOOPT-NOT: "-fsplit-machine-functions"