[Driver][X86] Add -mpad-max-prefix-size

[skan]

Summary

The option -mpad-max-prefix-size performs some checking and delegate to MC option -x86-pad-max-prefix-size. This option is designed for eliminate NOPs when we need to align something by adding redundant prefixes to instructions, e.g. it can be used along with -malign-branch, -malign-branch-boundary to prefix padding branch.

It has similar (but slightly different) effect as GAS's option -malign-branch-prefix-size, e.g. -mpad-max-prefix-size can also elminate NOPs emitted by align directive, so we use a different name here. I remove the option -malign-branch-prefix-size since is unimplemented and not needed. If we need to be compatible with GAS, we can make -malign-branch-prefix-size an alias for this option later.

[skan]

format the patch

[skan]

The remote build failed due to a bug of Pre-merge checks, which tried to apply a landed patch.

[skan]

Rebase

[LuoYuanke]

LGTM

[MaskRay]

It has similar (but slightly different) effect as GAS's option -malign-branch-prefix-size, e.g. -mpad-max-prefix-size can also elminate NOPs emitted by align directive, so we use a different name here. I remove the option -malign-branch-prefix-size since is unimplemented and not needed. If we need to be compatible with GAS, we can make -malign-branch-prefix-size an alias for this option later.

This looks like it requires more thorough discussions, doesn't it? We will also need discussions with the binutils side. We requested a GCC compiler driver option long ago but we do not reach a consensus (https://gcc.gnu.org/legacy-ml/gcc/2020-01/msg00358.html). IMHO the approval was in haste. With all due respect, I think I've seen such hasty approval (without any mentioning why such decisions are justifiable) and hasty commits in this area before several times, so I'll click "Request Changes" just in case of an accidental commit.

[LuoYuanke]

This looks like it requires more thorough discussions, doesn't it? We will also need discussions with the binutils side. We requested a GCC compiler driver option long ago but we do not reach a consensus (https://gcc.gnu.org/legacy-ml/gcc/2020-01/msg00358.html). IMHO the approval was in haste. With all due respect, I think I've seen such hasty approval (without any mentioning why such decisions are justifiable) and hasty commits in this area before several times, so I'll click "Request Changes" just in case of an accidental commit.

I accept the patch, because it just expose the LLVM option in clang and nobody reply on the patch. Actually we are waiting for you and Phillip to review it. If you have concern on the option. I'm open for discussion.

[skan]

In D77628#1969856, @MaskRay wrote:

This looks like it requires more thorough discussions, doesn't it? We will also need discussions with the binutils side. We requested a GCC compiler driver option long ago but we do not reach a consensus (https://gcc.gnu.org/legacy-ml/gcc/2020-01/msg00358.html). IMHO the approval was in haste. With all due respect, I think I've seen such hasty approval (without any mentioning why such decisions are justifiable) and hasty commits in this area before several times, so I'll click "Request Changes" just in case of an accidental commit.

AFAICS, this patch adds -mpad-max-prefix-size for clang, I mentioned -malign-branch-prefix-size in the summary just to clarify it has different behaviour from GAS's option -malign-branch-prefix-size. This patch has nothing to do with GCC compatibility, so I think the reasons for objection are not sufficient.

[skan]

Remove the limit "max padding prefix size <=5" since MC doesn't
have this limit.

[MaskRay]

clang/include/clang/Driver/Options.td

2190–2191

I think DriverOption is the default and you can omit it.

clang/include/clang/Driver/Options.td

 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 : CommaJoined<["-"], "malign-branch=">, Group<m_Group>, Flags<[DriverOption]>,
   HelpText<"Specify types of branches to align">;
 def malign_branch_boundary_EQ : Joined<["-"], "malign-branch-boundary=">, Group<m_Group>, Flags<[DriverOption]>,
   HelpText<"Specify the boundary's size to align branches">;
-def malign_branch_prefix_size_EQ : Joined<["-"], "malign-branch-prefix-size=">, Group<m_Group>;
+def mpad_max_prefix_size_EQ : Joined<["-"], "mpad-max-prefix-size=">, Group<m_Group>, Flags<[DriverOption]>,
+  HelpText<"Specify maximum number of prefixes to use for padding">;
 def mbranches_within_32B_boundaries : Flag<["-"], "mbranches-within-32B-boundaries">, Flags<[DriverOption]>, Group<m_Group>,
   HelpText<"Align selected branches (fused, jcc, jmp) within 32-byte boundary">;
 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 mdouble_EQ : Joined<["-"], "mdouble=">, Group<m_Group>, Values<"32,64">, Flags<[CC1Option]>,
   HelpText<"Force double to be 32 bits or 64 bits">;
 def LongDouble_Group : OptionGroup<"<LongDouble group>">, Group<m_Group>,

clang/lib/Driver/ToolChains/Clang.cpp

 //===-- Clang.cpp - Clang+LLVM ToolChain Implementations --------*- C++ -*-===//
 //
 // 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
 //
 //===----------------------------------------------------------------------===//
 
 #include "Clang.h"
for (StringRef T : A->getValues()) {
             << 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_malign_branch_prefix_size_EQ)) {
+          Args.getLastArg(options::OPT_mpad_max_prefix_size_EQ)) {
     StringRef Value = A->getValue();
     unsigned PrefixSize;
     if (Value.getAsInteger(10, PrefixSize) || PrefixSize > 5) {
       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-prefix-size=" +
+      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();

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

 // 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=; 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 -malign-branch-prefix-size=
-// RUN: %clang -target x86_64 -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-prefix-size=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-5
-// PREFIX-5: "-mllvm" "-x86-align-branch-prefix-size=5"
+/// 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=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-5
+// PREFIX-5: "-mllvm" "-x86-pad-max-prefix-size=5"
 
-// RUN: %clang -target x86_64 -malign-branch-prefix-size=6 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX-6
+// RUN: %clang -target x86_64 -mpad-max-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 -mbranches-within-32B-boundaries %s -c -### 2>&1 | FileCheck %s --check-prefix=32B
 // 32B: "-mllvm" "-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 -malign-branch-prefix-size=15 %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

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

 /// Test that -malign-branch* and -mbranches-within-32B-boundaries are handled for assembly files.
 
 // 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 -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-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 -mpad-max-prefix-size=5 %s -c -### 2>&1 | FileCheck %s --check-prefix=PREFIX
+// PREFIX: "-mllvm" "-x86-pad-max-prefix-size=5"
 
 // 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"