[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/Clang.cpp

void Clang::AddSystemZTargetArgs(const ArgList &Args,
   if (HasSoftFloat) {
     // Floating point operations and argument passing are soft.
     CmdArgs.push_back("-msoft-float");
     CmdArgs.push_back("-mfloat-abi");
     CmdArgs.push_back("soft");
   }
 }
 
-static void addX86AlignBranchArgs(const Driver &D, const ArgList &Args,
-                                  ArgStringList &CmdArgs) {
-  if (Args.hasArg(options::OPT_mbranches_within_32B_boundaries)) {
-    CmdArgs.push_back("-mllvm");
-    CmdArgs.push_back("-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("-mllvm");
-      CmdArgs.push_back(
-          Args.MakeArgString("-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("-mllvm");
-    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch=" + 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("-mllvm");
-      CmdArgs.push_back(
-          Args.MakeArgString("-x86-pad-max-prefix-size=" + Twine(PrefixSize)));
-    }
-  }
-}
-
 void Clang::AddX86TargetArgs(const ArgList &Args,
                              ArgStringList &CmdArgs) const {
   const Driver &D = getToolChain().getDriver();
-  addX86AlignBranchArgs(D, Args, CmdArgs);
+  addX86AlignBranchArgs(D, Args, CmdArgs, /*IsLTO=*/false);
 
   if (!Args.hasFlag(options::OPT_mred_zone, options::OPT_mno_red_zone, true) ||
       Args.hasArg(options::OPT_mkernel) ||
       Args.hasArg(options::OPT_fapple_kext))
     CmdArgs.push_back("-disable-red-zone");
 
   if (!Args.hasFlag(options::OPT_mtls_direct_seg_refs,
                     options::OPT_mno_tls_direct_seg_refs, true))
void ClangAs::AddMIPSTargetArgs(const ArgList &Args,
   mips::getMipsCPUAndABI(Args, Triple, CPUName, ABIName);
 
   CmdArgs.push_back("-target-abi");
   CmdArgs.push_back(ABIName.data());
 }
 
 void ClangAs::AddX86TargetArgs(const ArgList &Args,
                                ArgStringList &CmdArgs) const {
-  addX86AlignBranchArgs(getToolChain().getDriver(), Args, CmdArgs);
+  addX86AlignBranchArgs(getToolChain().getDriver(), Args, CmdArgs,
+                        /*IsLTO=*/false);
 
   if (Arg *A = Args.getLastArg(options::OPT_masm_EQ)) {
     StringRef Value = A->getValue();
     if (Value == "intel" || Value == "att") {
       CmdArgs.push_back("-mllvm");
       CmdArgs.push_back(Args.MakeArgString("-x86-asm-syntax=" + Value));
     } else {
       getToolChain().getDriver().Diag(diag::err_drv_unsupported_option_argument)

clang/lib/Driver/ToolChains/CommonArgs.h

SmallString<128> getStatsFileName(const llvm::opt::ArgList &Args,
                                   const InputInfo &Output,
                                   const InputInfo &Input, const Driver &D);
 
 /// \p Flag must be a flag accepted by the driver with its leading '-' removed,
 //     otherwise '-print-multi-lib' will not emit them correctly.
 void addMultilibFlag(bool Enabled, const char *const Flag,
                      Multilib::flags_list &Flags);
 
+void addX86AlignBranchArgs(const Driver &D, const llvm::opt::ArgList &Args,
+                           llvm::opt::ArgStringList &CmdArgs, bool IsLTO);
 } // end namespace tools
 } // end namespace driver
 } // end namespace clang
 
 #endif // LLVM_CLANG_LIB_DRIVER_TOOLCHAINS_COMMONARGS_H

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));
+
+  addX86AlignBranchArgs(D, Args, CmdArgs, /*IsLTO=*/true);
 }
 
 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;
 
   std::string CandidateRPath = TC.getArchSpecificLibPath();
SmallString<128> tools::getStatsFileName(const llvm::opt::ArgList &Args,
   llvm::sys::path::replace_extension(StatsFile, "stats");
   return StatsFile;
 }
 
 void tools::addMultilibFlag(bool Enabled, const char *const Flag,
                             Multilib::flags_list &Flags) {
   Flags.push_back(std::string(Enabled ? "+" : "-") + Flag);
 }
+
+void tools::addX86AlignBranchArgs(const Driver &D, const ArgList &Args,
+                                  ArgStringList &CmdArgs, bool IsLTO) {
+  auto addArg = [&, IsLTO](const Twine &Arg) {
+    if (IsLTO) {
+      CmdArgs.push_back(Args.MakeArgString("-plugin-opt=" + Arg));
+    } else {
+      CmdArgs.push_back("-mllvm");
+      CmdArgs.push_back(Args.MakeArgString(Arg));
+    }
+  };
+
+  if (Args.hasArg(options::OPT_mbranches_within_32B_boundaries)) {
+    addArg(Twine("-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 {
+      addArg("-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;
+    }
+    addArg("-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 {
+      addArg("-x86-pad-max-prefix-size=" + Twine(PrefixSize));
+    }
+  }
+}

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