[BOLT] Mark option values of --split-functions deprecated

[FPar]

Summary

The SplitFunctions pass does not distinguish between various splitting
modes anymore. This change updates the command line interface to
reflect this behavior by deprecating values passed to the
--split-function option.

[Amir]

bolt/lib/Passes/SplitFunctions.cpp

35

Non-boolean values are deprecated, but would they still be accepted?
Also, to make that warning stand out, please use WithColor::warning() (see example use in D126722).

[FPar]

Output warning to WithColor::warning()

[FPar]

bolt/lib/Passes/SplitFunctions.cpp

35

Yes, this patch does not change the behavior when passing "2" or "3" to -split-functions. The only visible change is the warning being emitted.

[rafauler]

That's awesome, thanks

You might want to wait if other reviewers have any more comments

[maksfb]

bolt/lib/Passes/SplitFunctions.cpp

35

We discussed the WithColor::warning() change offline and agreed it's a good idea to have it, but it has to be done in a consistent manner for all BOLT output. It's fine to keep errs() for now.

[FPar]

Revert WithColor::warning() to errs()

bolt/README.md

 the command line above.
 
 ### Step 3: Optimize with BOLT
 
 Once you have `perf.fdata` ready, you can use it for optimizations with
 BOLT. Assuming your environment is setup to include the right path, execute
 `llvm-bolt`:
 ```
-$ llvm-bolt <executable> -o <executable>.bolt -data=perf.fdata -reorder-blocks=ext-tsp -reorder-functions=hfsort -split-functions=2 -split-all-cold -split-eh -dyno-stats
+$ llvm-bolt <executable> -o <executable>.bolt -data=perf.fdata -reorder-blocks=ext-tsp -reorder-functions=hfsort -split-functions -split-all-cold -split-eh -dyno-stats
 ```
 
 If you do need an updated debug info, then add `-update-debug-sections` option
 to the command above. The processing time will be slightly longer.
 
 For a full list of options see `-help`/`-help-hidden` output.
 
 The input binary for this step does not have to 100% match the binary used for

bolt/docs/OptimizingClang.md

 ```bash
   $ perf2bolt $CPATH/clang-7 -p perf.data -o clang-7.fdata -w clang-7.yaml
 ```
 Notice that we are passing `clang-7` to `perf2bolt` which is the real binary that
 `clang` and `clang++` are symlinking to. The next step will optimize Clang using
 the generated profile:
 ```bash
 $ llvm-bolt $CPATH/clang-7 -o $CPATH/clang-7.bolt -b clang-7.yaml \
-    -reorder-blocks=ext-tsp -reorder-functions=hfsort+ -split-functions=3 \
+    -reorder-blocks=ext-tsp -reorder-functions=hfsort+ -split-functions \
     -split-all-cold -dyno-stats -icf=1 -use-gnu-stack
 ```
 The output will look similar to the one below:
 ```t
 ...
 BOLT-INFO: enabling relocation mode
 BOLT-INFO: 11415 functions out of 104526 simple functions (10.9%) have non-empty execution profile.
 ...

bolt/include/bolt/Passes/SplitFunctions.h

 #include "llvm/Support/CommandLine.h"
 #include <atomic>
 
 namespace llvm {
 namespace bolt {
 
 /// Split function code in multiple parts.
 class SplitFunctions : public BinaryFunctionPass {
-public:
-  /// Settings for splitting function bodies into hot/cold partitions.
-  enum SplittingType : char {
-    ST_NONE = 0, /// Do not split functions.
-    ST_LARGE,    /// In non-relocation mode, only split functions that
-                 /// are too large to fit into the original space.
-    ST_ALL,      /// Split all functions.
-  };
-
 private:
   /// Split function body into fragments.
   void splitFunction(BinaryFunction &Function);
 
   /// Create trampoline landing pads for exception handling code to guarantee
   /// that every landing pad is placed in the same function fragment as the
   /// corresponding thrower block. The trampoline landing pad, when created,
   /// will redirect the execution to the real landing pad in a different

bolt/lib/Passes/SplitFunctions.cpp

 //===- bolt/Passes/SplitFunctions.cpp - Pass for splitting function code --===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 //
 // This file implements the SplitFunctions pass.
 //
 //===----------------------------------------------------------------------===//
 
 #include "bolt/Passes/SplitFunctions.h"
 #include "bolt/Core/BinaryFunction.h"
 #include "bolt/Core/ParallelUtilities.h"
 #include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormatVariadic.h"
 
 #include <vector>
 
 #define DEBUG_TYPE "bolt-opts"
 
 using namespace llvm;
 using namespace bolt;
 
+namespace {
+class DeprecatedSplitFunctionOptionParser : public cl::parser<bool> {
+public:
+  explicit DeprecatedSplitFunctionOptionParser(cl::Option &O)
+      : cl::parser<bool>(O) {}
+
+  bool parse(cl::Option &O, StringRef ArgName, StringRef Arg, bool &Value) {
+    if (Arg == "2" || Arg == "3") {
+      Value = true;
+      errs() << formatv("BOLT-WARNING: specifying non-boolean value \"{0}\" "
+                        "for option -{1} is deprecated\n",
+                        Arg, ArgName);
+      return false;
+    }
+    return cl::parser<bool>::parse(O, ArgName, Arg, Value);
+  }
+};
+} // namespace
+
 namespace opts {
 
 extern cl::OptionCategory BoltOptCategory;
 
 extern cl::opt<bool> SplitEH;
 extern cl::opt<unsigned> ExecutionCountThreshold;
 
 static cl::opt<bool> AggressiveSplitting(
     "split-all-cold", cl::desc("outline as many cold basic blocks as possible"),
     cl::cat(BoltOptCategory));
 
 static cl::opt<unsigned> SplitAlignThreshold(
     "split-align-threshold",
     cl::desc("when deciding to split a function, apply this alignment "
              "while doing the size comparison (see -split-threshold). "
              "Default value: 2."),
     cl::init(2),
 
     cl::Hidden, cl::cat(BoltOptCategory));
 
-static cl::opt<SplitFunctions::SplittingType>
+static cl::opt<bool, false, DeprecatedSplitFunctionOptionParser>
     SplitFunctions("split-functions",
                    cl::desc("split functions into hot and cold regions"),
-  cl::init(SplitFunctions::ST_NONE),
-  cl::values(clEnumValN(SplitFunctions::ST_NONE, "0",
-                        "do not split any function"),
-             clEnumValN(SplitFunctions::ST_LARGE, "1",
-                        "in non-relocation mode only split functions too large "
-                        "to fit into original code space"),
-             clEnumValN(SplitFunctions::ST_LARGE, "2",
-                        "same as 1 (backwards compatibility)"),
-             clEnumValN(SplitFunctions::ST_ALL, "3",
-                        "split all functions")),
-  cl::ZeroOrMore,
                    cl::cat(BoltOptCategory));
 
 static cl::opt<unsigned> SplitThreshold(
     "split-threshold",
     cl::desc("split function only if its main size is reduced by more than "
              "given amount of bytes. Default value: 0, i.e. split iff the "
              "size is reduced. Note that on some architectures the size can "
              "increase after splitting."),
     cl::init(0), cl::Hidden, cl::cat(BoltOptCategory));
 
-void syncOptions(BinaryContext &BC) {
-  if (!BC.HasRelocations && opts::SplitFunctions == SplitFunctions::ST_LARGE)
-    opts::SplitFunctions = SplitFunctions::ST_ALL;
-}
-
 } // namespace opts
 
 namespace llvm {
 namespace bolt {
 
 bool SplitFunctions::shouldOptimize(const BinaryFunction &BF) const {
   // Apply execution count threshold
   if (BF.getKnownExecutionCount() < opts::ExecutionCountThreshold)
     return false;
 
   return BinaryFunctionPass::shouldOptimize(BF);
 }
 
 void SplitFunctions::runOnFunctions(BinaryContext &BC) {
-  opts::syncOptions(BC);
-
-  if (opts::SplitFunctions == SplitFunctions::ST_NONE)
+  if (!opts::SplitFunctions)
     return;
 
   ParallelUtilities::WorkFuncTy WorkFun = [&](BinaryFunction &BF) {
     splitFunction(BF);
   };
 
   ParallelUtilities::PredicateTy SkipFunc = [&](const BinaryFunction &BF) {
     return !shouldOptimize(BF);
void SplitFunctions::splitFunction(BinaryFunction &BF) {
   size_t HotSize;
   size_t ColdSize;
   if (BC.isX86()) {
     std::tie(OriginalHotSize, ColdSize) = BC.calculateEmittedSize(BF);
     LLVM_DEBUG(dbgs() << "Estimated size for function " << BF
                       << " pre-split is <0x"
                       << Twine::utohexstr(OriginalHotSize) << ", 0x"
                       << Twine::utohexstr(ColdSize) << ">\n");
-    if (opts::SplitFunctions == SplitFunctions::ST_LARGE &&
-        !BC.HasRelocations) {
-      // Split only if the function wouldn't fit.
-      if (OriginalHotSize <= BF.getMaxSize())
-        return;
-    }
   }
 
   // Never outline the first basic block.
   BF.layout_front()->setCanOutline(false);
   for (BinaryBasicBlock *BB : BF.layout()) {
     if (!BB->canOutline())
       continue;
     if (BB->getExecutionCount() != 0) {

bolt/test/X86/bug-reorder-bb-jrcxz.s

 
 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown \
 # RUN:   %s -o %t.o
 # RUN: link_fdata %s %t.o %t.fdata
 # RUN: %clang %cflags %t.o -falign-labels -march=native -o %t.exe -Wl,-q
 
 # RUN:  llvm-bolt %t.exe -o %t.bolted --data %t.fdata \
 # RUN:    --reorder-blocks=ext-tsp --reorder-functions=hfsort \
-# RUN:    --split-functions=2 --split-all-cold --split-eh --dyno-stats \
+# RUN:    --split-functions --split-all-cold --split-eh --dyno-stats \
 # RUN:    --print-finalized 2>&1 | FileCheck %s
 
 # CHECK-NOT: value of -2105 is too large for field of 1 byte.
 
   .text
   .section .text.startup,"ax",@progbits
   .p2align 5,,31
   .globl main

bolt/test/X86/jump-table-icp.test

 RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown \
 RUN:   %p/Inputs/jump_table_icp.s -o %t.o
 RUN: link_fdata %p/Inputs/jump_table_icp.s %t.o %t.fdata --nmtool llvm-nm
 RUN: llvm-strip --strip-unneeded %t.o
 RUN: %clang %cflags -no-pie %t.o -o %t.exe -Wl,-q
 
 RUN: (llvm-bolt %t.exe --data %t.fdata -o %t --relocs \
-RUN:   --reorder-blocks=cache --split-functions=3 --split-all-cold \
+RUN:   --reorder-blocks=cache --split-functions --split-all-cold \
 RUN:   --use-gnu-stack --dyno-stats --indirect-call-promotion=jump-tables \
 RUN:   --print-icp -v=0 \
 RUN:   --icp-jt-remaining-percent-threshold=10 \
 RUN:   --icp-jt-total-percent-threshold=2 \
 RUN:   --indirect-call-promotion-topn=1 \
 RUN:   --icp-jump-tables-targets --align-functions-max-bytes=7 2>&1 && \
 RUN:   llvm-objdump -d %t --print-imm-hex) | FileCheck %s
 

bolt/test/X86/shared_object_entry.s

 # RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown %s -o %t.o
 # RUN: ld.lld %t.o -o %t.so --shared --entry=func1.cold.1 --emit-relocs
 # RUN: llvm-bolt -relocs %t.so -o %t -reorder-functions=hfsort+ \
-# RUN:    -split-functions=3 -reorder-blocks=ext-tsp -split-all-cold \
+# RUN:    -split-functions -reorder-blocks=ext-tsp -split-all-cold \
 # RUN:    -dyno-stats -icf=1 -use-gnu-stack
 
 # Check that an entry point is a cold symbol
 # RUN: llvm-readelf -h %t.so > %t.log
 # RUN: llvm-nm %t.so >> %t.log
 # RUN: FileCheck %s --input-file %t.log
 # CHECK: Entry point address: 0x[[#%X,ENTRY:]]
 # CHECK: [[#%x,ENTRY]] {{.*}} func1.cold.1

bolt/test/X86/unreachable.test

 # Check unreachable code elimination
 
 RUN: %clang %cflags %p/../Inputs/stub.c -fPIC -pie -shared -o %t.so
 RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown \
 RUN:   %p/Inputs/unreachable.s -o %t.o
 RUN: %clangxx %cxxflags -no-pie %t.o -o %t.exe %t.so
 RUN: llvm-bolt %t.exe -o %t \
-RUN:   -reorder-blocks=none -split-functions=1 -eliminate-unreachable \
+RUN:   -reorder-blocks=none -split-functions -eliminate-unreachable \
 RUN:   -funcs=foo -use-gnu-stack -print-cfg -print-finalized \
 RUN:   | FileCheck %s --check-prefix=BOLT
 RUN: llvm-objdump -d %t --print-imm-hex --disassemble-symbols=foo \
 RUN:   --no-show-raw-insn | FileCheck %s
 
 BOLT: BB Layout   : .LBB{{.*}}, .LFT{{.*}}, .Ltmp{{.*}}, .LFT{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .LLP{{.*}}, .LFT{{.*}}, .Ltmp{{.*}}, .LFT{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}
 BOLT: BOLT-INFO: UCE removed 4 blocks and 16 bytes of code.
 BOLT: BB Layout   : .LBB{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .LLP{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}, .Ltmp{{.*}}

bolt/test/runtime/X86/exceptions-instrumentation.test

 # Check instrumentation of C++ code with exceptions
 
 REQUIRES: system-linux
 
 RUN: %clangxx %cxxflags -no-pie %p/Inputs/exceptions_split.cpp -Wl,-q -g -o %t_exc_split
 RUN: llvm-bolt %t_exc_split -o %t.exc --instrument --instrumentation-file=%t.fdata
 
 RUN: %t.exc arg1 arg2 arg3
 
 RUN: llvm-bolt %t_exc_split -o %t.exc.bolted --data %t.fdata \
 RUN:   --reorder-blocks=ext-tsp --reorder-functions=hfsort+ \
-RUN:   --split-functions=3 --split-eh=1 \
+RUN:   --split-functions --split-eh=1 \
 RUN:   | FileCheck --check-prefix=EXCEPTIONS %s
 EXCEPTIONS-NOT: invalid (possibly stale) profile
 
 # Check that cold symbols are emitted with local binding.
 RUN: llvm-readelf -Ws %t.exc.bolted | FileCheck %s --check-prefix=CHECK-READELF
 CHECK-READELF: FUNC    LOCAL  DEFAULT   {{.*}} main.cold.{{.*}}
 
 RUN: cat %t.fdata | FileCheck --check-prefix=EXCEPTIONS-FDATA %s
 # Check that the printf "catch" ran exactly 10 times
 EXCEPTIONS-FDATA: 1 main {{.*}} 1 _Znwm@PLT 0 0 10

bolt/test/runtime/X86/pie-exceptions-split.test

 ## Check that BOLT successfully splits C++ exception-handling code for
 ## PIEs or shared objects.
 
 REQUIRES: system-linux
 
 RUN: %clangxx %cxxflags -pie -fPIC %p/Inputs/exceptions_split.cpp -Wl,-q -o %t
 RUN: llvm-bolt %t -o %t.instr --instrument --instrumentation-file=%t.fdata
 
 ## Record profile with invocation that does not throw exceptions.
 RUN: %t.instr
 
 RUN: llvm-bolt %t -o %t.bolt --data %t.fdata --reorder-blocks=ext-tsp \
-RUN:   --split-functions=1 --split-eh --print-after-lowering \
+RUN:   --split-functions --split-eh --print-after-lowering \
 RUN:   --print-only=main 2>&1 | FileCheck %s
 
 ## All calls to printf() should be from exception handling code that was
 ## recorded as cold during the profile collection run. Check that the calls
 ## are placed after the split point.
 CHECK-NOT: callq printf
 CHECK: HOT-COLD SPLIT POINT
 CHECK:     callq printf
 
 ## Verify the output still executes correctly when the exception path is being
 ## taken.
 RUN: %t.bolt arg1 arg2 arg3 2>&1 | FileCheck --check-prefix=CHECK-BOLTED %s
 
 CHECK-BOLTED:      catch 2
 CHECK-BOLTED-NEXT: catch 1
-

bolt/test/runtime/meta-merge-fdata.test

 # Check unmapped calls coverage - libc should be calling _start via
 # uninstrumented code
 RUN: cat %t.fdata1 | FileCheck %s --check-prefix=CHECK-FDATA
 CHECK-FDATA: 0 [unknown] 0 1 _start 0 0 1
 
 # Check that BOLT works with this profile
 RUN: llvm-bolt merge-fdata -o %t.bolt --data %t.fdata1 \
 RUN:   --reorder-blocks=ext-tsp --reorder-functions=hfsort+ \
-RUN:   --split-functions=3 \
+RUN:   --split-functions \
 RUN:   | FileCheck %s --check-prefix=CHECK-BOLT1
 CHECK-BOLT1-NOT: invalid (possibly stale) profile
 
 # Execute again
 RUN: %t.inst --help
 RUN: mv %t.fdata %t.fdata2
 
 # Check profile coverage
 RUN: llvm-boltdiff merge-fdata merge-fdata \
 RUN:   --data %t.fdata1 --data2 %t.fdata2 --display-count=20 | \
 RUN:   FileCheck %s --check-prefix=CHECK-BOLTDIFF
 CHECK-BOLTDIFF: Inputs share [[#]] functions
 
 # Check that instrumented binary produces the same result
 RUN: merge-fdata %t.fdata1 %t.fdata2 > %t.fdata.base
 RUN: %t.inst %t.fdata1 %t.fdata2 > %t.fdata.inst
 RUN: cmp %t.fdata.base %t.fdata.inst
 
 # Optimize using merged fdata
 RUN: llvm-bolt merge-fdata -o %t.opt --data %t.fdata.base \
 RUN:   --reorder-blocks=ext-tsp --reorder-functions=hfsort+ \
-RUN:   --split-functions=3 \
+RUN:   --split-functions \
 RUN:   | FileCheck %s --check-prefix=CHECK-BOLT2
 CHECK-BOLT2-NOT: invalid (possibly stale) profile
 
 # Check that optimized binary produces the same result
 RUN: %t.opt %t.fdata1 %t.fdata2 > %t.fdata.opt
 RUN: cmp %t.fdata.base %t.fdata.opt