[Driver][X86] Add -malign-branch* and -mbranches-within-32B-boundaries

[MaskRay]

Summary

These driver options perform some checking and delegate to MC options -x86-align-branch* and -x86-branches-within-32B-boundaries.

[merge_guards_bot]

Unit tests: pass. 61683 tests passed, 0 failed and 779 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

@skan Does this look good to you?

[MaskRay]

-malign-branch-boundary= is the primary option. So swap the roles

[skan]

In D72227 you wrote

I think options should follow these principles:

1. Different options are position independent. -mA -mB should be the same as -mB -mA.
2. -mA and -mno-A are position dependent and the last one wins. Sometimes, the set may include more than 2 options, e.g. the last of -fno-pic -fpie and -fpic wins.
3. More specific options can override semantics of less specific options. In our case, -malign-branch* are more specific than -malign-branch-within-32B-boundaries.

This implemetation is consistent with the principles. However, as I mentioned in D72227, I hold a slightly different opinion about the principles.

1. Different options are position independent. -mA -mB should be the same as -mB -mA.

I think only when A and B are not related, (-mA -mB) == (-mB -mA).

2. -mA and -mno-A are position dependent and the last one wins. Sometimes, the set may include more than 2 options, e.g. the last of -fno-pic -fpie and -fpic wins

Totally agree.

3. More specific options can override semantics of less specific options. In our case, -malign-branch* are more specific than -malign-branch-within-32B-boundaries.

I think general options can aslo override specific options. The last one wins. e.g.
The net effect of -malign-branch=fused -malign-branch-within-32B-boundaries should be -malign-branch-boundary=32 -malign-branch=fused+jcc+jmp -malign-branch-prefix-size=5

clang/lib/Driver/ToolChains/Clang.cpp

2034

AlignBranch = StringRef() seems unnecessary.

2040

Any reason for precluding 16?

2053

AlignBranch = StringRef() seems unnecessary.

[merge_guards_bot]

Unit tests: pass. 61743 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

Delete a AlignBranch = StringRef()

[skan]

Add revison D72225 as parent?

clang/lib/Driver/ToolChains/Clang.cpp

2028

Any reason for precluding 16?

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

2040

I did not preclude 16.

2053

This comment was for the previous diff.

[merge_guards_bot]

Unit tests: fail. 61741 tests passed, 2 failed and 780 were skipped.

failed: Clang.Driver/x86-malign-branch.c
failed: Clang.Driver/x86-malign-branch.s

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

The commutative property (independence of options) makes option composition easier. clangDriver makes heavy use of getLastArg and hasArg. Without the commutative property, it would now be able to shuffle code around.

The -malign-branch=jmp -mno-branches-within-32B-boundaries issue does not matter that much. I do not expect -mno-branches-within-32B-boundaries to be used much. If we want to disable branch alignment, -malign-branch-boundary=0 can be used.

[MaskRay]

In D72463#1813658, @skan wrote:

Add revison D72225 as parent?

-x86-align-branch-prefix-size= does not work without D72225, but I think the two patches can be independent. GCC does not support these options and no projects rely on the option yet.

[skan]

clang/lib/Driver/ToolChains/Clang.cpp

2040

I am sorry that I misunderstood the word "preclude". As far as I know, we would like AlignBranchBoundary>=32

[skan]

In D72463#1813669, @MaskRay wrote:

The commutative property (independence of options) makes option composition easier. clangDriver makes heavy use of getLastArg and hasArg. Without the commutative property, it would now be able to shuffle code around.

The -malign-branch=jmp -mno-branches-within-32B-boundaries issue does not matter that much. I do not expect -mno-branches-within-32B-boundaries to be used much. If we want to disable branch alignment, -malign-branch-boundary=0 can be used.

The behaviour of your proposal is "specific option is always override the gerneral option, no matter which is the last". I still prefer not to support the override, which makes things more clear. But I am fine if other reviewers believe this behaviour is reasonable. @jyknight @reames @craig.topper

clang/include/clang/Basic/DiagnosticDriverKinds.td

254

The error information "must be one of: " is kind of misleading.
It seems that one kind of branch can be aligned.

[skan]

clang/include/clang/Basic/DiagnosticDriverKinds.td

254

The error information "must be one of: " is kind of misleading.
It seems that "only" one kind of branch can be aligned.

[MaskRay]

Delete -mno-branches-within-32B-boundaries (GNU as does not support it)
Use comma joined list instead of +. CommaJoined is more conventional. For example, -mrecip= and -fsanitize= a comma separated list.

[MaskRay]

clang/include/clang/Basic/DiagnosticDriverKinds.td

254

Added each element

clang/lib/Driver/ToolChains/Clang.cpp

2040

GNU as allows -malign-branch-boundary=16. I think we should allow it as well. It may not be optimal, but it does not hurt to give the user more choices.

[merge_guards_bot]

Unit tests: pass. 61771 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[MaskRay]

Make driver a thin wrapper since we are going to add a master option in MC (D72738)

[merge_guards_bot]

Unit tests: fail. 61862 tests passed, 1 failed and 781 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class/try_lock.pass.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

[skan]

LGTM
The principle of override here is consistent with MC (D72738). So I think this patch is better than D72227.

[annita.zhang]

@MaskRay Did you merge it to LLVM 10 branch?

[MaskRay]

In D72463#1826499, @annita.zhang wrote:

@MaskRay Did you merge it to LLVM 10 branch?

It is included in the release branch.

git branch origin/release/10.x --contains 5ca24d09aefaedf8e4148c7fce4b4ab0c4ecc72a # suceeded

[annita.zhang]

In D72463#1826821, @MaskRay wrote:

In D72463#1826499, @annita.zhang wrote:

@MaskRay Did you merge it to LLVM 10 branch?

It is included in the release branch.

git branch origin/release/10.x --contains 5ca24d09aefaedf8e4148c7fce4b4ab0c4ecc72a # suceeded

Thank you!

clang/include/clang/Basic/DiagnosticDriverKinds.td

 def warn_drv_omp_offload_target_missingbcruntime : Warning<
   "No library '%0' found in the default clang lib directory or in LIBRARY_PATH. Expect degraded performance due to no inlining of runtime functions on target devices.">,
   InGroup<OpenMPTarget>;
 def err_drv_unsupported_embed_bitcode
     : Error<"%0 is not supported with -fembed-bitcode">;
 def err_drv_bitcode_unsupported_on_toolchain : Error<
   "-fembed-bitcode is not supported on versions of iOS prior to 6.0">;
 
+def err_drv_invalid_malign_branch_EQ : Error<
+  "invalid argument '%0' to -malign-branch=; must be one of: %1">;
+
 def warn_O4_is_O3 : Warning<"-O4 is equivalent to -O3">, InGroup<Deprecated>;
 def warn_drv_optimization_value : Warning<"optimization level '%0' is not supported; using '%1%2' instead">,
   InGroup<InvalidCommandLineArgument>;
 def warn_ignored_gcc_optimization : Warning<"optimization flag '%0' is not supported">,
   InGroup<IgnoredOptimizationArgument>;
 def warn_ignored_clang_option : Warning<"the flag '%0' has been deprecated and will be ignored">,
   InGroup<UnusedCommandLineArgument>;
 def warn_drv_unsupported_opt_for_target : Warning<

clang/include/clang/Driver/Options.td

 def mx32 : Flag<["-"], "mx32">, Group<m_Group>, Flags<[DriverOption, CoreOption]>;
 def mabi_EQ : Joined<["-"], "mabi=">, Group<m_Group>;
 def miamcu : Flag<["-"], "miamcu">, Group<m_Group>, Flags<[DriverOption, CoreOption]>,
   HelpText<"Use Intel MCU ABI">;
 def mno_iamcu : Flag<["-"], "mno-iamcu">, Group<m_Group>, Flags<[DriverOption, CoreOption]>;
 def malign_functions_EQ : Joined<["-"], "malign-functions=">, Group<clang_ignored_m_Group>;
 def malign_loops_EQ : Joined<["-"], "malign-loops=">, Group<clang_ignored_m_Group>;
 def malign_jumps_EQ : Joined<["-"], "malign-jumps=">, Group<clang_ignored_m_Group>;
+def malign_branch_EQ : Joined<["-"], "malign-branch=">, Group<m_Group>;
+def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group<m_Group>;
+def malign_branch_prefix_size_EQ : Joined<["-"], "malign-branch-prefix-size=">, Group<m_Group>;
+def mbranches_within_32B_boundaries : Joined<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group<m_Group>;
+def mno_branches_within_32B_boundaries : Joined<["-"], "mno-branches-within-32B-boundaries">, Flags<[DriverOption]>, Group<m_Group>;
 def mfancy_math_387 : Flag<["-"], "mfancy-math-387">, Group<clang_ignored_m_Group>;
 def mlong_calls : Flag<["-"], "mlong-calls">, Group<m_Group>,
   HelpText<"Generate branches with extended addressability, usually via indirect jumps.">;
 def LongDouble_Group : OptionGroup<"<LongDouble group>">, Group<m_Group>,
   DocName<"Long double flags">,
   DocBrief<[{Selects the long double implementation}]>;
 def mlong_double_64 : Flag<["-"], "mlong-double-64">, Group<LongDouble_Group>, Flags<[CC1Option]>,
   HelpText<"Force long double to be 64 bits">;

clang/lib/Driver/ToolChains/Clang.cpp

D.Diag(diag::err_drv_unsupported_opt)
       " " + Args.getLastArg(options::OPT_mbackchain)->getAsString(Args);
   }
   if (HasBackchain)
     CmdArgs.push_back("-mbackchain");
   if (HasPackedStack)
     CmdArgs.push_back("-mpacked-stack");
 }
 
+static void addX86AlignBranchesArgs(const Driver &D, const ArgList &Args,
+  ArgStringList &CmdArgs) {
+  unsigned AlignBranchBoundary = 0, AlignBranchPrefixSize = 0;
+  std::string AlignBranch;
+  if (Args.hasFlag(options::OPT_mbranches_within_32B_boundaries,
+                   options::OPT_mno_branches_within_32B_boundaries, false)) {
+    AlignBranch = "fused+jcc+jmp";
+    AlignBranchBoundary = 32;
+    AlignBranchPrefixSize = 5;
+  }
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_EQ)) {
+    SmallVector<StringRef, 0> BranchTypes;
+    AlignBranch = A->getValue();
+    StringRef(AlignBranch).split(BranchTypes, '+', -1, false);
+    for (StringRef T : BranchTypes)
+      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";
+        AlignBranch = StringRef();
+      }
+  }
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_boundary_EQ)) {
+    StringRef Value = A->getValue();
+    if (Value.getAsInteger(10, AlignBranchBoundary) ||
+        AlignBranchBoundary < 16 || !llvm::isPowerOf2_64(AlignBranchBoundary)) {
+      D.Diag(diag::err_drv_invalid_argument_to_option)
+          << Value << A->getOption().getName();
+      AlignBranch = StringRef();
+    }
+  }
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_malign_branch_prefix_size_EQ)) {
+    StringRef Value = A->getValue();
+    if (Value.getAsInteger(10, AlignBranchPrefixSize) ||
+        AlignBranchPrefixSize > 5) {
+      D.Diag(diag::err_drv_invalid_argument_to_option)
+          << Value << A->getOption().getName();
+      AlignBranch = StringRef();
+    }
+  }
+  if (!AlignBranch.empty()) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch=" + AlignBranch));
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-boundary=" +
+                                         Twine(AlignBranchBoundary)));
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-prefix-size=" +
+                                         Twine(AlignBranchPrefixSize)));
+  }
+}
+
 void Clang::AddX86TargetArgs(const ArgList &Args,
                              ArgStringList &CmdArgs) const {
+  const Driver &D = getToolChain().getDriver();
+  addX86AlignBranchesArgs(D, Args, CmdArgs);
+
   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))
     CmdArgs.push_back("-mno-tls-direct-seg-refs");
if (NoImplicitFloat)
     CmdArgs.push_back("-no-implicit-float");
 
   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)
+      D.Diag(diag::err_drv_unsupported_option_argument)
           << A->getOption().getName() << Value;
     }
-  } else if (getToolChain().getDriver().IsCLMode()) {
+  } else if (D.IsCLMode()) {
     CmdArgs.push_back("-mllvm");
     CmdArgs.push_back("-x86-asm-syntax=intel");
   }
 
   // Set flags to support MCU ABI.
   if (Args.hasFlag(options::OPT_miamcu, options::OPT_mno_iamcu, false)) {
     CmdArgs.push_back("-mfloat-abi");
     CmdArgs.push_back("soft");
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 {
+  addX86AlignBranchesArgs(getToolChain().getDriver(), Args, CmdArgs);
+
   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)
           << A->getOption().getName() << Value;

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

+/// Test that -malign-branch* options are parsed and converted to -mllvm options.
+
+/// 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 -malign-branch=fused+foo+bar %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE-ERR
+// TYPE-ERR: invalid argument 'foo' to -malign-branch=; must be one of: fused, jcc, jmp, call, ret, indirect
+// TYPE-ERR: invalid argument 'bar' to -malign-branch=; must be one of: fused, jcc, jmp, call, ret, indirect
+
+/// Test -malign-branch-boundary=
+// RUN: %clang -target x86_64 -malign-branch=jcc -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 -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-prefix-size=
+// RUN: %clang -target x86_64 -malign-branch=jcc -malign-branch-prefix-size=0 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-0
+// PREFIX-0: "-mllvm" "-x86-align-branch-prefix-size=0"
+// RUN: %clang -target x86_64 -malign-branch=jcc -malign-branch-prefix-size=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-5
+// PREFIX-5: "-mllvm" "-x86-align-branch-prefix-size=5"
+
+// RUN: %clang -target x86_64 -malign-branch-prefix-size=6 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-6
+// PREFIX-6: invalid argument
+
+/// Test -mbranches-within-32B-boundaries
+// RUN: %clang -target x86_64 -mno-branches-within-32B-boundaries -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B
+// 32B:  "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch-prefix-size=5"
+// RUN: %clang -target x86_64 -mbranches-within-32B-boundaries -mno-branches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=NO32B
+// NO32B-NOT: "-x86-align-branch
+/// Test that -malign-branch* can override individual values of -mbranches-within-32B-boundaries
+// RUN: %clang -target x86_64 -malign-branch=jcc -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B-TYPE
+// 32B-TYPE:  "-mllvm" "-x86-align-branch=jcc" "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch-prefix-size=5"
+
+/// 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 -malign-branch-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

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

+/// Test that -malign-branch* are handled for assembly files.
+
+// RUN: %clang -target x86_64 -malign-branch=fused+jcc+jmp %s -c -### %s 2>&1 | FileCheck %s --check-prefix=TYPE
+// TYPE: "-mllvm" "-x86-align-branch=fused+jcc+jmp"
+
+// RUN: %clang -target x86_64 -malign-branch=jcc -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 -malign-branch=jcc -malign-branch-prefix-size=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX
+// PREFIX: "-mllvm" "-x86-align-branch-prefix-size=5"
+
+// RUN: %clang -target x86_64 -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B
+// 32B: "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch-prefix-size=5"