upporting -f(no)-reorder-functions flag, clang side change

[twoh]

Summary

When profile data is given, .hot/.unlikely section prefix is
added to hot/cold functions for linker to improve code locality. GCC
controls this behavior with '-f(no)-reorder-functions' flag, while LLVM
uses opt tool option '-profile-guided-section-prefix=true/false'. This
patch is for LLVM to support the same driver flag with GCC. LLVM side
patch is D34795.

[twoh]

Ping. Thanks!

[davidxl]

The patch is missing documentation change.

[twoh]

Update documentation. Please let me know if I need to update other documents as well. Thanks!

[davidxl]

The patch itself is fine. The meta question is whether we expect this option to be generally useful?

[twoh]

I think it is generally good to match what GCC does to not to confuse people.

[twoh]

Friendly ping. @davidxl, I think there's no harm to make clang consistent with gcc for compiler options, and I wonder if you have any concerns that I may miss. Thanks!

[davidxl]

docs/ClangCommandLineReference.rst

1955

prefix or suffix? Or just leave the details out (also consider the interaction with -ffunction-sections)?

Consider documenting:

1. what is the default behavior?
2. how does it affect function ordering?

[xbolva00]

Ping

[twoh]

Rebase. Sorry I somehow missed the recent comments. I addresses @davidxl's comment on documentation. Thanks!

[joerg]

Excuse me for bring this up so late, but why do we want to make any such promises? As in: fundamentally, LLVM IR doesn't have any order property on the module level. I have yet so seen reasonable code where the order of functions matters for anything but performance. I've seen a few things that required -funit-at-a-time, most noticable GCC's CRT implementation. But those are all major hacks. So under what premise is it useful to have to even pretend to honor source code order?

[twoh]

@joerg Sorry but I'm not sure if I understand your question. This doesn't pretend to honor source code order, but makes linker to place "hot" functions under .text.hot section (There's no guarantee of ordering between functions inside .hot.text section) while "cold" functions under .text.unlikely section. This is purely for performance.

[twoh]

Remove conflict line.

[srhines]

test/Driver/function-sections.c

77

There should ideally be a test for the default behavior as well (and please consider mentioning the default behavior in the commit message).

[lebedev.ri]

docs/ClangCommandLineReference.rst

1953

Isn't this autogenerated from include/clang/Driver/Options.td?

docs/ClangCommandLineReference.rst

 .. option:: -ftrigraphs, -fno-trigraphs, -trigraphs, --trigraphs
 
 Process trigraph sequences
 
 .. option:: -funique-section-names, -fno-unique-section-names
 
 Use unique names for text and data sections (ELF Only)
 
+.. option:: -freorder-functions, -fno-reorder-functions
+
+Enable function reordering for better code locality. When enabled, hot/cold functions will be placed under .text.hot/.text.unlikely subsection in the final binary, respectively. If -ffunction-section is provided, this will append .text.hot/.text.unlikely prefix to the section name. This is turned on by default, but only effective when profile data is provided.
+
 .. option:: -funit-at-a-time, -fno-unit-at-a-time
 
 .. option:: -funroll-loops, -fno-unroll-loops
 
 Turn on loop unroller
 
 .. option:: -funsafe-math-optimizations, -fno-unsafe-math-optimizations
 

include/clang/Driver/Options.td

 def fno_preserve_as_comments : Flag<["-"], "fno-preserve-as-comments">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Do not preserve comments in inline assembly">;
 def fprofile_arcs : Flag<["-"], "fprofile-arcs">, Group<f_Group>;
 def fno_profile_arcs : Flag<["-"], "fno-profile-arcs">, Group<f_Group>;
 def framework : Separate<["-"], "framework">, Flags<[LinkerInput]>;
 def frandom_seed_EQ : Joined<["-"], "frandom-seed=">, Group<clang_ignored_f_Group>;
 def freg_struct_return : Flag<["-"], "freg-struct-return">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Override the default ABI to return small structs in registers">;
+def freorder_functions : Flag <["-"], "freorder-functions">,
+  Group<f_Group>, Flags<[CC1Option]>,
+  HelpText<"Using special subsections for hot/cold functions to improve code "
+           "locality">;
+def fno_reorder_functions : Flag <["-"], "fno-reorder-functions">,
+  Group<f_Group>, Flags<[CC1Option]>;
 def frtti : Flag<["-"], "frtti">, Group<f_Group>;
 def : Flag<["-"], "fsched-interblock">, Group<clang_ignored_f_Group>;
 def fshort_enums : Flag<["-"], "fshort-enums">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Allocate to an enum type only as many bytes as it needs for the declared range of possible values">;
 def fchar8__t : Flag<["-"], "fchar8_t">, Group<f_Group>, Flags<[CC1Option]>,
   HelpText<"Enable C++ builtin type char8_t">;
 def fno_char8__t : Flag<["-"], "fno-char8_t">, Group<f_Group>,
   HelpText<"Disable C++ builtin type char8_t">;

include/clang/Frontend/CodeGenOptions.def

 CODEGENOPT(SoftFloat         , 1, 0) ///< -soft-float.
 CODEGENOPT(SpeculativeLoadHardening, 1, 0) ///< Enable speculative load hardening.
 CODEGENOPT(FineGrainedBitfieldAccesses, 1, 0) ///< Enable fine-grained bitfield accesses.
 CODEGENOPT(StrictEnums       , 1, 0) ///< Optimize based on strict enum definition.
 CODEGENOPT(StrictVTablePointers, 1, 0) ///< Optimize based on the strict vtable pointers
 CODEGENOPT(TimePasses        , 1, 0) ///< Set when -ftime-report is enabled.
 CODEGENOPT(UnrollLoops       , 1, 0) ///< Control whether loops are unrolled.
 CODEGENOPT(RerollLoops       , 1, 0) ///< Control whether loops are rerolled.
+CODEGENOPT(ReorderFunctions  , 1, 1) ///< Set when -freorder-functions is enabled.
 CODEGENOPT(NoUseJumpTables   , 1, 0) ///< Set when -fno-jump-tables is enabled.
 CODEGENOPT(UnsafeFPMath      , 1, 0) ///< Allow unsafe floating point optzns.
 CODEGENOPT(UnwindTables      , 1, 0) ///< Emit unwind tables.
 CODEGENOPT(VectorizeLoop     , 1, 0) ///< Run loop vectorizer.
 CODEGENOPT(VectorizeSLP      , 1, 0) ///< Run SLP vectorizer.
 CODEGENOPT(ProfileSampleAccurate, 1, 0) ///< Sample profile is accurate.
 
   /// Attempt to use register sized accesses to bit-fields in structures, when

lib/CodeGen/BackendUtil.cpp

static void initTargetOptions(llvm::TargetOptions &Options,
   Options.NoInfsFPMath = CodeGenOpts.NoInfsFPMath;
   Options.NoNaNsFPMath = CodeGenOpts.NoNaNsFPMath;
   Options.NoZerosInBSS = CodeGenOpts.NoZeroInitializedInBSS;
   Options.UnsafeFPMath = CodeGenOpts.UnsafeFPMath;
   Options.StackAlignmentOverride = CodeGenOpts.StackAlignment;
   Options.FunctionSections = CodeGenOpts.FunctionSections;
   Options.DataSections = CodeGenOpts.DataSections;
   Options.UniqueSectionNames = CodeGenOpts.UniqueSectionNames;
+  Options.ReorderFunctions = CodeGenOpts.ReorderFunctions;
   Options.EmulatedTLS = CodeGenOpts.EmulatedTLS;
   Options.ExplicitEmulatedTLS = CodeGenOpts.ExplicitEmulatedTLS;
   Options.DebuggerTuning = CodeGenOpts.getDebuggerTuning();
   Options.EmitStackSizeSection = CodeGenOpts.StackSizeSection;
   Options.EmitAddrsig = CodeGenOpts.Addrsig;
 
   if (CodeGenOpts.EnableSplitDwarf)
     Options.MCOptions.SplitDwarfFile = CodeGenOpts.SplitDwarfFile;

lib/Driver/ToolChains/Clang.cpp

if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
                    UseSeparateSections)) {
     CmdArgs.push_back("-fdata-sections");
   }
 
   if (!Args.hasFlag(options::OPT_funique_section_names,
                     options::OPT_fno_unique_section_names, true))
     CmdArgs.push_back("-fno-unique-section-names");
 
+  if (!Args.hasFlag(options::OPT_freorder_functions,
+                    options::OPT_fno_reorder_functions, true))
+    CmdArgs.push_back("-fno-reorder-functions");
+
   if (auto *A = Args.getLastArg(
       options::OPT_finstrument_functions,
       options::OPT_finstrument_functions_after_inlining,
       options::OPT_finstrument_function_entry_bare))
     A->render(Args, CmdArgs);
 
   // NVPTX 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
#endif
   }
 
   // Pass the path to compiler resource files.
   CmdArgs.push_back("-resource-dir");
   CmdArgs.push_back(D.ResourceDir.c_str());
 
   Args.AddLastArg(CmdArgs, options::OPT_working_directory);
 
+  bool ARCMTEnabled = false;
+  if (!Args.hasArg(options::OPT_fno_objc_arc, options::OPT_fobjc_arc)) {
+    if (const Arg *A = Args.getLastArg(options::OPT_ccc_arcmt_check,
+                                       options::OPT_ccc_arcmt_modify,
+                                       options::OPT_ccc_arcmt_migrate)) {
+      ARCMTEnabled = true;
+      switch (A->getOption().getID()) {
+      default:
+        llvm_unreachable("missed a case");
+      case options::OPT_ccc_arcmt_check:
+        CmdArgs.push_back("-arcmt-check");
+        break;
+      case options::OPT_ccc_arcmt_modify:
+        CmdArgs.push_back("-arcmt-modify");
+        break;
+      case options::OPT_ccc_arcmt_migrate:
+        CmdArgs.push_back("-arcmt-migrate");
+        CmdArgs.push_back("-mt-migrate-directory");
+        CmdArgs.push_back(A->getValue());
+
+        Args.AddLastArg(CmdArgs, options::OPT_arcmt_migrate_report_output);
+        Args.AddLastArg(CmdArgs, options::OPT_arcmt_migrate_emit_arc_errors);
+        break;
+      }
+    }
+  } else {
+    Args.ClaimAllArgs(options::OPT_ccc_arcmt_check);
+    Args.ClaimAllArgs(options::OPT_ccc_arcmt_modify);
+    Args.ClaimAllArgs(options::OPT_ccc_arcmt_migrate);
+  }
+
+  if (const Arg *A = Args.getLastArg(options::OPT_ccc_objcmt_migrate)) {
+    if (ARCMTEnabled) {
+      D.Diag(diag::err_drv_argument_not_allowed_with) << A->getAsString(Args)
+                                                      << "-ccc-arcmt-migrate";
+    }
+    CmdArgs.push_back("-mt-migrate-directory");
+    CmdArgs.push_back(A->getValue());
+
+    if (!Args.hasArg(options::OPT_objcmt_migrate_literals,
+                     options::OPT_objcmt_migrate_subscripting,
+                     options::OPT_objcmt_migrate_property)) {
+      // None specified, means enable them all.
+      CmdArgs.push_back("-objcmt-migrate-literals");
+      CmdArgs.push_back("-objcmt-migrate-subscripting");
+      CmdArgs.push_back("-objcmt-migrate-property");
+    } else {
+      Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_literals);
+      Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_subscripting);
+      Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_property);
+    }
+  } else {
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_literals);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_subscripting);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_property);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_all);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_readonly_property);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_readwrite_property);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_property_dot_syntax);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_annotation);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_instancetype);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_nsmacros);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_protocol_conformance);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_atomic_property);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_returns_innerpointer_property);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_ns_nonatomic_iosonly);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_migrate_designated_init);
+    Args.AddLastArg(CmdArgs, options::OPT_objcmt_whitelist_dir_path);
+  }
+
+  // Pass the path to compiler resource files.
+  CmdArgs.push_back("-resource-dir");
+  CmdArgs.push_back(D.ResourceDir.c_str());
+
+  Args.AddLastArg(CmdArgs, options::OPT_working_directory);
+
   RenderARCMigrateToolOptions(D, Args, CmdArgs);
 
   // Add preprocessing options like -I, -D, etc. if we are using the
   // preprocessor.
   //
   // FIXME: Support -fpreprocessed
   if (types::getPreprocessedType(InputType) != types::TY_INVALID)
     AddPreprocessingOptions(C, JA, D, Args, CmdArgs, Output, Inputs);

lib/Frontend/CompilerInvocation.cpp

static bool ParseCodeGenArgs(CodeGenOptions &Opts, ArgList &Args, InputKind IK,
   Opts.FunctionSections = Args.hasFlag(OPT_ffunction_sections,
                                        OPT_fno_function_sections, false);
   Opts.DataSections = Args.hasFlag(OPT_fdata_sections,
                                    OPT_fno_data_sections, false);
   Opts.StackSizeSection =
       Args.hasFlag(OPT_fstack_size_section, OPT_fno_stack_size_section, false);
   Opts.UniqueSectionNames = Args.hasFlag(OPT_funique_section_names,
                                          OPT_fno_unique_section_names, true);
+  Opts.ReorderFunctions =
+      Args.hasFlag(OPT_freorder_functions, OPT_fno_reorder_functions, true);
 
   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);

test/CodeGen/Inputs/freorder-functions.prof

+hot_func:1000:0
+ 1: 0
+cold_func:0:0
+ 1: 1
+ 2: 1

test/CodeGen/freorder-functions.c

+// REQUIRES: x86-registered-target
+
+// RUN: %clang_cc1 -triple x86_64-pc-linux -S -O3 -ffunction-sections -fprofile-sample-use=%S/Inputs/freorder-functions.prof -o - < %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux -S -O3 -ffunction-sections -fprofile-sample-use=%S/Inputs/freorder-functions.prof -fno-reorder-functions -o - < %s | FileCheck --check-prefix=CHECK-NOPREFIX %s
+
+// opt tool option precedes driver option.
+// RUN: %clang_cc1 -triple x86_64-pc-linux -S -O3 -ffunction-sections -fprofile-sample-use=%S/Inputs/freorder-functions.prof -fno-reorder-functions -mllvm -profile-guided-section-prefix=true -o - < %s | FileCheck %s
+// RUN: %clang_cc1 -triple x86_64-pc-linux -S -O3 -ffunction-sections -fprofile-sample-use=%S/Inputs/freorder-functions.prof -freorder-functions -mllvm -profile-guided-section-prefix=false -o - < %s | FileCheck --check-prefix=CHECK-NOPREFIX %s
+
+void hot_func() {
+  return;
+}
+
+void cold_func() {
+  hot_func();
+  return;
+}
+
+// CHECK: .section .text.hot.hot_func,"ax",@progbits
+// CHECK: .section .text.unlikely.cold_func,"ax",@progbits
+// CHECK-NOPREFIX: .section .text.hot_func,"ax",@progbits
+// CHECK-NOPREFIX: .section .text.cold_func,"ax",@progbits

test/Driver/function-sections.c

 // Test handling of -f(no-)function-sections and -f(no-)data-sections
 //
 // CHECK-FS: -ffunction-sections
 // CHECK-NOFS-NOT: -ffunction-sections
 // CHECK-DS: -fdata-sections
 // CHECK-NODS-NOT: -fdata-sections
 // CHECK-US-NOT: -fno-unique-section-names
 // CHECK-NOUS: -fno-unique-section-names
+// CHECK-RF-NOT: -fno-reorder-functions
+// CHECK-NORF: -fno-reorder-functions
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1       \
 // RUN:     -target i386-unknown-linux \
 // RUN:   | FileCheck --check-prefix=CHECK-NOFS --check-prefix=CHECK-NODS %s
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1       \
 // RUN:     -target i386-unknown-linux \
 // RUN:     -ffunction-sections \

 // RUN:     -target i386-unknown-linux \
 // RUN:     -funique-section-names \
 // RUN:   | FileCheck --check-prefix=CHECK-US %s
 
 // RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1        \
 // RUN:     -target i386-unknown-linux \
 // RUN:     -fno-unique-section-names \
 // RUN:   | FileCheck --check-prefix=CHECK-NOUS %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1        \
+// RUN:     -target i386-unknown-linux \
+// RUN:     -freorder-functions \
+// RUN:   | FileCheck --check-prefix=CHECK-RF %s
+
+// RUN: %clang -no-canonical-prefixes %s -### -fsyntax-only 2>&1        \
+// RUN:     -target i386-unknown-linux \
+// RUN:     -fno-reorder-functions \
+// RUN:   | FileCheck --check-prefix=CHECK-NORF %s