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

[skan]

Summary

Add -mbranches-within-32B-boundaries to align branches within a 32-Byte
boundary to reduce the potential performance loss of the microcode
update. It is equivalent to the combination of three options:

-malign-branch=fused,jcc,jmp
-malign-branch-boundary=32
-malign-branch-prefix-size=5

-malign-branch* and -mbranches-within-32B-boundaries override each
other, the last one wins. e.g

The net effect of -mbranches-within-32B-boundaries -malign-branch=jcc
is:
-malign-branch=jcc
-malign-branch-boundary=32
-malign-branch-prefix-size=5

The net effect of -malign-branch=jcc -mbranches-within-32B-boundaries
is:
-malign-branch=fused+jcc+jmp
-malign-branch-boundary=32
-malign-branch-prefix-size=5

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

2015

The convention is camelCase. This file is inconsistent but some newly added functions follow this convention.

2017

  if (!TC.getTriple().isX86()) {
    D.Diag(diag::err_drv_unsupported_opt_for_target)
        << A->getAsString(Args) << TripleStr;
  return;
}

I will add Triple::isX86 in D72247.

2050

OPT_mbranches_within_32B_boundaries should provide default values which can be overridden by more specific options.

clang/test/Driver/intel-align-branch.c

29

Since you have a -mno.. -m.. test, the -m.. test is redundant.

36

Add a -target aarch64 test that other targets will get an error.

(I am lazy and usually just write -target x86_64 (generic ELF) and omit -unknown-known)

[skan]

clang/lib/Driver/ToolChains/Clang.cpp

2017

Is this check necessary? alignBranchesOptions is only called by Clang::AddX86TargetArgs and ClangAs::AddX86TargetArgs, so the triple is always X86.

If -target aarch64 is passed, -mbranches-within-32B-boundaries will not be consumed and user can get an warning. So is -malign-branch etc.

clang-10: warning: argument unused during compilation: '-mbranches-within-32B-boundaries' [-Wunused-command-line-argument]

2050

Currently, -mbranches-within-32B-boundaries is equivalent to `-malign-branch-boundary=32 -malign-branch=fused+jcc+jmp -malign-branch-prefix-size=4

What is expected behaviour would be very confusing if specific options could override -mbranches-within-32B-boundaries. For example, if passed options are

-mbranches-within-32B-boundaries -malign-branch-boundary=32 -mno-branches-within-32B-boundaries

What should the value of -malign-branch-boundary be? Is it 32 or 0?

If we think -mno-branches-within-32B-boundaries is the negative form of -mbranches-within-32B-boundaries , then -malign-branch-boundary should be 32.

Or if we think -mno-branches-within-32B-boundaries wins since it appears at the end, and -mno-branches-within-32B-boundaries means no need to align branches, -malign-branch-boundary should be 0.

As long as we don't support specific options could override -mbranches-within-32B-boundaries, the trouble disappears :-)

clang/test/Driver/intel-align-branch.c

29

Done

36

Currently, if -target aarch64 is passed to clang, we will get a warning

clang-10: warning: argument unused during compilation: '-mbranches-within-32B-boundaries' [-Wunused-command-line-argument]

Is this prompt not clear enough?

[annita.zhang]

A general comment. All the tests have been done with -malign-branch-prefix-size=5. I don't see any explicit reason to change the default prefix size from 5 to 4.

[annita.zhang]

clang/include/clang/Driver/Options.td

2201

"aligned to prevent from being across or against the boundary of specified size."

2209

"types). The branches' types are combination of jcc, fused, "

2211–2214

"jcc indicates conditional jumps, fused indicates fused conditional jumps,"
"jmp indicates unconditional jumps, call indicates direct and indirect calls,"
"ret indicates rets, indirect indicates indirect jumps."

2220

Is there an upbound for this parameter?

2230

-malign-branch-prefix-size=5.">;

[annita.zhang]

clang/lib/Driver/ToolChains/Clang.cpp

2057

CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-prefix-size=5"));

[skan]

clang/include/clang/Driver/Options.td

2220

yes, both upbound and default value is 5

[MaskRay]

clang/include/clang/Driver/Options.td

2203

-x86-align-branch-boundary=0

We should not expose llc internal option names.

clang/lib/Driver/ToolChains/Clang.cpp

2050

-mbranches-within-32B-boundaries -malign-branch-boundary=32 -mno-branches-within-32B-boundaries

My preference is that the net effect will be: -malign-branch-boundary=32

If (Args.hasFlag(options::OPT_mbranches_within_32B_boundaries, options::OPT_mno_branches_within_32B_boundaries, false))
  boundary = 32;
if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_EQ))
  boundary = ...
if (boundary)
  add -mllvm boundary

but I'd like to hear what others say. @jyknight @reames

clang/test/Driver/intel-align-branch.c

36

You need a test on a different architecture to check that the warning is emitted as intended.

[MaskRay]

clang/include/clang/Driver/Options.td

2196

Group<m_x86_Features_Group>

2200

"It should be 0 or a power of 2 no less than 16."

Do you want to preclude 16?

Long descriptions should be placed in DocBrief values.

2203

"The default value 0 does not align branches."

2234

HelpText<"Opposite to -mbranches-within-32B-boundaries.">;

Delete. No need to elaborate.

[craig.topper]

clang/include/clang/Driver/Options.td

2196

Doesn't m_x86_Features_Group make things get collected for -target-features? I don't think this should be part of that.

[MaskRay]

Created D72463 (-mbranches-within-32B-boundaries can be overridden by -malign-branch*; added test coverage)

[LuoYuanke]

clang/lib/Driver/ToolChains/Clang.cpp

2050

I have no preference. What's the general rule for such case in LLVM? Is there any similar option design before?

[MaskRay]

clang/lib/Driver/ToolChains/Clang.cpp

2050

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.

I have implemented these ideas in https://reviews.llvm.org/D72463. I don't include documentation. Maybe documentation can be added in a different change (for example, this one, if D72463 looks good to you).

[skan]

clang/include/clang/Driver/Options.td

2200

No, I don't want to preclude 16.

[skan]

clang/lib/Driver/ToolChains/Clang.cpp

2050

More specific options can override semantics of less specific options.

I hold a slightly different opinion about this. I think only when specific option appears at the right of the general option, override can happen.

[skan]

The override principle of driver should keep same with MC (D72738). D72463 is better, so I prefer to abandon this revision.

clang/include/clang/Driver/Options.td

 def mmcu_EQ : Joined<["-"], "mmcu=">, Group<m_Group>;
 def mdynamic_no_pic : Joined<["-"], "mdynamic-no-pic">, Group<m_Group>;
 def mfix_and_continue : Flag<["-"], "mfix-and-continue">, Group<clang_ignored_m_Group>;
 def mieee_fp : Flag<["-"], "mieee-fp">, Group<clang_ignored_m_Group>;
 def minline_all_stringops : Flag<["-"], "minline-all-stringops">, Group<clang_ignored_m_Group>;
 def mno_inline_all_stringops : Flag<["-"], "mno-inline-all-stringops">, Group<clang_ignored_m_Group>;
 def malign_double : Flag<["-"], "malign-double">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Align doubles to two words in structs (x86 only)">;
+def malign_branch_boundary_EQ
+    : Joined<["-"], "malign-branch-boundary=">,
+      Group<m_Group>,
+      Flags<[DriverOption, HelpHidden]>,
+      HelpText<
+          "Control how the assembler should align branches with NOP or segment "
+          "override prefix. If the boundary's size is not 0, it should be a "
+          "power of 2 and no less than 32. Branches will be aligned to prevent "
+          "from being across or against the boundary of specified size. "
+          "-malign-branch-boundary=0 doesn't align branches.">;
+def malign_branch_EQ
+    : Joined<["-"], "malign-branch=">,
+      Group<m_Group>,
+      Flags<[DriverOption, HelpHidden]>,
+      HelpText<"Specify types of branches to align (plus separated list of "
+               "types). The branches's types are combination of jcc, fused, "
+               "jmp, call, ret, indirect. jcc indicates conditional jumps, "
+               "fused indicates fused conditional jumps, jmp indicates "
+               "unconditional jumps, call indicates direct and indirect calls, "
+               "ret indicates rets, indirect indicates indirect jumps.">;
+def malign_branch_prefix_size_EQ
+    : Joined<["-"], "malign-branch-prefix-size=">,
+      Group<m_Group>,
+      Flags<[DriverOption, CC1Option, HelpHidden]>,
+      HelpText<"Specify the maximum number of prefixes on an instruction to "
+               "align branches. The number should be between 0 and 5.">;
+def mbranches_within_32B_boundaries
+    : Flag<["-"], "mbranches-within-32B-boundaries">,
+      Group<m_Group>,
+      Flags<[DriverOption]>,
+      HelpText<
+          "Aligns conditional jumps, fused conditional jumps, and "
+          "unconditional "
+          "jumps within 32 byte boundary with up to 5 segment prefixes on an "
+          "instruction. It is equivalent to -malign-branch-boundary=32, "
+          "-malign-branch=fused+jcc+jmp, -malign-branch-prefix-size=5.">;
+def mno_branches_within_32B_boundaries
+    : Flag<["-"], "mno-branches-within-32B-boundaries">,
+      Group<m_Group>,
+      Flags<[DriverOption]>,
+      HelpText<"Opposite to -mbranches-within-32B-boundaries.">;
 def mfloat_abi_EQ : Joined<["-"], "mfloat-abi=">, Group<m_Group>, Values<"soft,softfp,hard">;
 def mfpmath_EQ : Joined<["-"], "mfpmath=">, Group<m_Group>;
 def mfpu_EQ : Joined<["-"], "mfpu=">, Group<m_Group>;
 def mhwdiv_EQ : Joined<["-"], "mhwdiv=">, Group<m_Group>;
 def mhwmult_EQ : Joined<["-"], "mhwmult=">, Group<m_Group>;
 def mglobal_merge : Flag<["-"], "mglobal-merge">, Group<m_Group>, Flags<[CC1Option]>,
   HelpText<"Enable merging of globals">;
 def mhard_float : Flag<["-"], "mhard-float">, Group<m_Group>;

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 alignBranchesOptions(const Driver &D, const ArgList &Args,
+                                 ArgStringList &CmdArgs) {
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_boundary_EQ)) {
+    StringRef Value = A->getValue();
+    uint64_t Num;
+    if (!Value.getAsInteger(10, Num) && Num >= 32 && llvm::isPowerOf2_64(Num)) {
+      CmdArgs.push_back("-mllvm");
+      CmdArgs.push_back(
+          Args.MakeArgString("-x86-align-branch-boundary=" + Value));
+    } else {
+      D.Diag(diag::err_drv_unsupported_option_argument)
+          << A->getOption().getName() << Value;
+    }
+  }
+
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_EQ)) {
+    StringRef Value = A->getValue();
+    SmallVector<StringRef, 6> BranchTypes;
+    Value.split(BranchTypes, '+', -1, false);
+    for (auto BranchType : BranchTypes) {
+      if (BranchType != "fused" && BranchType != "jcc" && BranchType != "jmp" &&
+          BranchType != "call" && BranchType != "ret" &&
+          BranchType != "indirect")
+        D.Diag(diag::err_drv_unsupported_option_argument)
+            << A->getOption().getName() << Value;
+    }
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch=" + Value));
+  }
+
+  if (const Arg *A =
+          Args.getLastArg(options::OPT_malign_branch_prefix_size_EQ)) {
+    StringRef Value = A->getValue();
+    uint8_t Num;
+    if (!Value.getAsInteger(10, Num) && Num <= 5) {
+      CmdArgs.push_back("-mllvm");
+      CmdArgs.push_back(
+          Args.MakeArgString("-x86-align-branch-prefix-size=" + Value));
+    } else {
+      D.Diag(diag::err_drv_unsupported_option_argument)
+          << A->getOption().getName() << Value;
+    }
+  }
+
+  if (Args.hasFlag(options::OPT_mbranches_within_32B_boundaries,
+                   options::OPT_mno_branches_within_32B_boundaries, false)) {
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-boundary=32"));
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch=fused+jcc+jmp"));
+    CmdArgs.push_back("-mllvm");
+    CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-prefix-size=5"));
+  }
+}
+
 void Clang::AddX86TargetArgs(const ArgList &Args,
                              ArgStringList &CmdArgs) const {
+  alignBranchesOptions(getToolChain().getDriver(), 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");
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 {
+  alignBranchesOptions(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/intel-align-branch.c

+// RUN: %clang -target x86_64-unknown-unknown -malign-branch-boundary=32 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-BOUNDARY
+// CHECK-BOUNDARY: "-mllvm" "-x86-align-branch-boundary=32"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=jcc -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-JCC
+// CHECK-JCC: "-mllvm" "-x86-align-branch=jcc"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=fused -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-FUSED
+// CHECK-FUSED: "-mllvm" "-x86-align-branch=fused"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=jmp -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-JMP
+// CHECK-JMP: "-mllvm" "-x86-align-branch=jmp"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=call -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-CALL
+// CHECK-CALL: "-mllvm" "-x86-align-branch=call"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=ret -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-RET
+// CHECK-RET: "-mllvm" "-x86-align-branch=ret"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=indirect -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-INDIRECT
+// CHECK-INDIRECT: "-mllvm" "-x86-align-branch=indirect"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=fused+jcc+jmp+ret+call+indirect -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-BRANCH
+// CHECK-BRANCH: "-mllvm" "-x86-align-branch=fused+jcc+jmp+ret+call+indirect"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch-prefix-size=4 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-PREFIX
+// CHECK-PREFIX: "-mllvm" "-x86-align-branch-prefix-size=4"
+//
+// RUN: %clang -target x86_64-unknown-unknown -mno-branches-within-32B-boundaries -mbranches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-TOTAL1
+// CHECK-TOTAL1: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=5"
+//
+// RUN: %clang -target x86_64-unknown-unknown -mbranches-within-32B-boundaries -mno-branches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-TOTAL2
+// CHECK-TOTAL2-NOT: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=5"
+//
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch-boundary=7 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch=jump -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+// RUN: %clang -target x86_64-unknown-unknown -malign-branch-prefix-size=15 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-ERROR
+// CHECK-ERROR: error: unsupported argument
+//
+// RUN: %clang -target aarch64-unknown-unknown -malign-branch-boundary=7 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-WARNING
+// RUN: %clang -target aarch64-unknown-unknown -malign-branch=jump -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-WARNING
+// RUN: %clang -target aarch64-unknown-unknown -malign-branch-prefix-size=15 -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-WARNING
+// RUN: %clang -target aarch64-unknown-unknown -mbranches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-WARNING
+// CHECK-WARNING: warning: argument unused