[Driver][X86] Support branch align options with LTO

[skan]

Summary

Before this patch, we use two different ways to pass options to align branch depending on whether LTO is enabled. For example, -mbranches-within-32B-boundaries w/o LTO and -Wl,-plugin-opt=-x86-branches-within-32B-boundaries w/ LTO. It's inconvenient, so this patch unifies the way: we only need to pass options like -mbranches-within-32B-boundaries to align branches, no matter LTO is enabled or not.

[MaskRay]

clang/test/Driver/lto.c

81 ↗

(On Diff #265204)

This piece of logic is closer to x86. The test can be added to x86-malign-branch.c instead

[MaskRay]

clang/lib/Driver/ToolChains/CommonArgs.cpp

501

The large chunk of code duplicates Clang.cpp:addX86AlignBranchArgs.

Can you refactor addX86AlignBranchArgs to be reused here?

[skan]

Move tests to x86-malign-branch.c

[skan]

Refactor addX86AlignBranchArgs to be reused in CommonArgs.cpp

[skan]

Ping.

[MaskRay]

In D80289#2051947, @skan wrote:

Ping.

https://reviews.llvm.org/D80168#2046093

"Unless something is urgent, the usual practice is to ping after a week rather than 24 hours, as many people have big piles on their review plate, so can't always get to it the next day."

clang/test/Driver/x86-malign-branch.c

6

Drop -unknown-linux

ditto below

[skan]

clang/test/Driver/x86-malign-branch.c

6

Sorry it seems I didn't submit my comment...

Dropping -unknown-linux here would get an error

error: 'x86_64': unable to pass LLVM bit-code files to linker

[MaskRay]

clang/test/Driver/x86-malign-branch.c

6

Ah, you are right.

clang/lib/Driver/ToolChains/CommonArgs.cpp

 bool tools::isUseSeparateSections(const llvm::Triple &Triple) {
   return Triple.getOS() == llvm::Triple::CloudABI;
 }
 
 void tools::addLTOOptions(const ToolChain &ToolChain, const ArgList &Args,
                           ArgStringList &CmdArgs, const InputInfo &Output,
                           const InputInfo &Input, bool IsThinLTO) {
   const char *Linker = Args.MakeArgString(ToolChain.GetLinkerPath());
+  const Driver &D = ToolChain.getDriver();
   if (llvm::sys::path::filename(Linker) != "ld.lld" &&
       llvm::sys::path::stem(Linker) != "ld.lld") {
     // Tell the linker to load the plugin. This has to come before
     // AddLinkerInputs as gold requires -plugin to come before any -plugin-opt
     // that -Wl might forward.
     CmdArgs.push_back("-plugin");
 
 #if defined(_WIN32)
     const char *Suffix = ".dll";
 #elif defined(__APPLE__)
     const char *Suffix = ".dylib";
 #else
     const char *Suffix = ".so";
 #endif
 
     SmallString<1024> Plugin;
-    llvm::sys::path::native(Twine(ToolChain.getDriver().Dir) +
-                                "/../lib" CLANG_LIBDIR_SUFFIX "/LLVMgold" +
-                                Suffix,
-                            Plugin);
+    llvm::sys::path::native(
+        Twine(D.Dir) + "/../lib" CLANG_LIBDIR_SUFFIX "/LLVMgold" + Suffix,
     CmdArgs.push_back(Args.MakeArgString(Plugin));
   }
 
   // Try to pass driver level flags relevant to LTO code generation down to
   // the plugin.
 
   // Handle flags for selecting CPU variants.
   std::string CPU = getCPUName(Args, ToolChain.getTriple());
if (Args.hasArg(options::OPT_gsplit_dwarf)) {
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-plugin-opt=dwo_dir=") +
             Output.getFilename() + "_dwo"));
   }
 
   if (IsThinLTO)
     CmdArgs.push_back("-plugin-opt=thinlto");
 
-  StringRef Parallelism = getLTOParallelism(Args, ToolChain.getDriver());
+  StringRef Parallelism = getLTOParallelism(Args, D);
   if (!Parallelism.empty())
     CmdArgs.push_back(
         Args.MakeArgString("-plugin-opt=jobs=" + Twine(Parallelism)));
 
   // If an explicit debugger tuning argument appeared, pass it along.
   if (Arg *A = Args.getLastArg(options::OPT_gTune_Group,
                                options::OPT_ggdbN_Group)) {
     if (A->getOption().matches(options::OPT_glldb))
#endif
   if (Args.hasFlag(options::OPT_fdata_sections, options::OPT_fno_data_sections,
                    UseSeparateSections)) {
     CmdArgs.push_back("-plugin-opt=-data-sections");
   }
 
   if (Arg *A = getLastProfileSampleUseArg(Args)) {
     StringRef FName = A->getValue();
     if (!llvm::sys::fs::exists(FName))
-      ToolChain.getDriver().Diag(diag::err_drv_no_such_file) << FName;
+      D.Diag(diag::err_drv_no_such_file) << FName;
     else
       CmdArgs.push_back(
           Args.MakeArgString(Twine("-plugin-opt=sample-profile=") + FName));
   }
 
   auto *CSPGOGenerateArg = Args.getLastArg(options::OPT_fcs_profile_generate,
                                            options::OPT_fcs_profile_generate_EQ,
                                            options::OPT_fno_profile_generate);
#endif
   // Need this flag to turn on new pass manager via Gold plugin.
   if (Args.hasFlag(options::OPT_fexperimental_new_pass_manager,
                    options::OPT_fno_experimental_new_pass_manager,
                    /* Default */ ENABLE_EXPERIMENTAL_NEW_PASS_MANAGER)) {
     CmdArgs.push_back("-plugin-opt=new-pass-manager");
   }
 
   // Setup statistics file output.
-  SmallString<128> StatsFile =
-      getStatsFileName(Args, Output, Input, ToolChain.getDriver());
+  SmallString<128> StatsFile = getStatsFileName(Args, Output, Input, D);
   if (!StatsFile.empty())
     CmdArgs.push_back(
         Args.MakeArgString(Twine("-plugin-opt=stats-file=") + StatsFile));
+
+  // Handle options for branch align
+  if (Args.hasArg(options::OPT_mbranches_within_32B_boundaries)) {
+    CmdArgs.push_back("-plugin-opt=-x86-branches-within-32B-boundaries");
+  }
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_boundary_EQ)) {
+    StringRef Value = A->getValue();
+    unsigned Boundary;
+    if (Value.getAsInteger(10, Boundary) || Boundary < 16 ||
+        !llvm::isPowerOf2_64(Boundary)) {
+      D.Diag(diag::err_drv_invalid_argument_to_option)
+          << Value << A->getOption().getName();
+    } else {
+      CmdArgs.push_back(Args.MakeArgString(
+          "-plugin-opt=-x86-align-branch-boundary=" + Twine(Boundary)));
+    }
+  }
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_EQ)) {
+    std::string AlignBranch;
+    for (StringRef T : A->getValues()) {
+      if (T != "fused" && T != "jcc" && T != "jmp" && T != "call" &&
+          T != "ret" && T != "indirect")
+        D.Diag(diag::err_drv_invalid_malign_branch_EQ)
+            << T << "fused, jcc, jmp, call, ret, indirect";
+      if (!AlignBranch.empty())
+        AlignBranch += '+';
+      AlignBranch += T;
+    }
+    CmdArgs.push_back(Args.MakeArgString("-plugin-opt=-x86-align-branch=" +
+                                         Twine(AlignBranch)));
+  }
+  if (const Arg *A = Args.getLastArg(options::OPT_mpad_max_prefix_size_EQ)) {
+    StringRef Value = A->getValue();
+    unsigned PrefixSize;
+    if (Value.getAsInteger(10, PrefixSize)) {
+      D.Diag(diag::err_drv_invalid_argument_to_option)
+          << Value << A->getOption().getName();
+    } else {
+      CmdArgs.push_back(Args.MakeArgString(
+          "-plugin-opt=-x86-pad-max-prefix-size=" + Twine(PrefixSize)));
+    }
+  }
 }
 
 void tools::addArchSpecificRPath(const ToolChain &TC, const ArgList &Args,
                                  ArgStringList &CmdArgs) {
   if (!Args.hasFlag(options::OPT_frtlib_add_rpath,
                     options::OPT_fno_rtlib_add_rpath, false))
     return;
 

clang/test/Driver/x86-malign-branch.c

-/// Test that -malign-branch* and -mbranches-within-32B-boundaries are parsed and converted to -mllvm options.
+/// Test that -malign-branch* and -mbranches-within-32B-boundaries are parsed and converted to MC options.
 
 /// Test -malign-branch-boundary=
 // RUN: %clang -target x86_64 -malign-branch-boundary=16 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY
 // BOUNDARY: "-mllvm" "-x86-align-branch-boundary=16"
+// RUN: %clang -target x86_64-unknown-linux -malign-branch-boundary=16 -flto %s -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY-LTO
+// BOUNDARY-LTO: "-plugin-opt=-x86-align-branch-boundary=16"
 
 // RUN: %clang -target x86_64 -malign-branch-boundary=8 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY-ERR
 // RUN: %clang -target x86_64 -malign-branch-boundary=15 %s -c -### 2>&1 | FileCheck %s --check-prefix=BOUNDARY-ERR
 // BOUNDARY-ERR: invalid argument {{.*}} to -malign-branch-boundary=
 
 /// Test -malign-branch=
 // RUN: %clang -target x86_64 -malign-branch=fused,jcc,jmp %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE0
 // TYPE0: "-mllvm" "-x86-align-branch=fused+jcc+jmp"
 // RUN: %clang -target x86_64 -malign-branch=fused,jcc,jmp,ret,call,indirect %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE1
 // TYPE1: "-mllvm" "-x86-align-branch=fused+jcc+jmp+ret+call+indirect"
+// RUN: %clang -target x86_64-unknown-linux -malign-branch=fused,jcc,jmp -flto %s -### %s 2>&1 | FileCheck %s --check-prefix=TYPE0-LTO
+// TYPE0-LTO: "-plugin-opt=-x86-align-branch=fused+jcc+jmp"
 
 // RUN: %clang -target x86_64 -malign-branch=fused,foo,bar %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE-ERR
 // TYPE-ERR: invalid argument 'foo' to -malign-branch=; each element must be one of: fused, jcc, jmp, call, ret, indirect
 // TYPE-ERR: invalid argument 'bar' to -malign-branch=; each element must be one of: fused, jcc, jmp, call, ret, indirect
 
 /// Test -mpad-max-prefix-size=
 // RUN: %clang -target x86_64 -mpad-max-prefix-size=0 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-0
 // PREFIX-0: "-mllvm" "-x86-pad-max-prefix-size=0"
 // RUN: %clang -target x86_64 -mpad-max-prefix-size=15 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-15
 // PREFIX-15: "-mllvm" "-x86-pad-max-prefix-size=15"
+// RUN: %clang -target x86_64-unknown-linux -mpad-max-prefix-size=0 -flto %s -### 2>&1 | FileCheck %s --check-prefix=PREFIX-0-LTO
+// PREFIX-0-LTO: "-plugin-opt=-x86-pad-max-prefix-size=0"
 
 /// Test -mbranches-within-32B-boundaries
 // RUN: %clang -target x86_64 -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B
 // 32B: "-mllvm" "-x86-branches-within-32B-boundaries"
+// RUN: %clang -target x86_64-unknown-linux -mbranches-within-32B-boundaries -flto %s -### 2>&1 | FileCheck %s --check-prefix=32B-LTO
+// 32B-LTO: "-plugin-opt=-x86-branches-within-32B-boundaries"
 
 /// Unsupported on other targets.
 // RUN: %clang -target aarch64 -malign-branch=jmp %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
 // RUN: %clang -target aarch64 -malign-branch-boundary=7 %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
 // RUN: %clang -target aarch64 -mpad-max-prefix-size=15 %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
 // RUN: %clang -target aarch64 -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck --check-prefix=UNUSED %s
 // UNUSED: warning: argument unused