[PATCH] D72227: Add options for clang to align branches within 32B boundary

[Kan Shengchen via Phabricator via cfe-commits]

skan added inline comments.


================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2012
+                                 ArgStringList &CmdArgs) {
+  if (const Arg *A = Args.getLastArg(options::OPT_malign_branch_boundary_EQ)) {
+    StringRef Value = A->getValue();
----------------
MaskRay wrote:
>     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.
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]




================
Comment at: clang/lib/Driver/ToolChains/Clang.cpp:2045
+
+  if (Args.hasFlag(options::OPT_mbranches_within_32B_boundaries,
+                   options::OPT_mno_branches_within_32B_boundaries, false)) {
----------------
MaskRay wrote:
> `OPT_mbranches_within_32B_boundaries` should provide default values which can be overridden by more specific options.
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  :-)




================
Comment at: clang/test/Driver/intel-align-branch.c:28
+//
+// // RUN: %clang -target x86_64-unknown-unknown -mbranches-within-32B-boundaries -### -c %s 2>&1 | FileCheck %s --check-prefix=CHECK-TOTAL
+// CHECK-TOTAL: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=4"
----------------
MaskRay wrote:
> Since you have a `-mno.. -m..` test, the `-m..` test is redundant.
Done


================
Comment at: clang/test/Driver/intel-align-branch.c:35
+// 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-TOTAL3
+// CHECK-TOTAL3-NOT: "-mllvm" "-x86-align-branch-boundary=32" "-mllvm" "-x86-align-branch=fused+jcc+jmp" "-mllvm" "-x86-align-branch-prefix-size=4"
----------------
MaskRay wrote:
> 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`)
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?



CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D72227/new/

https://reviews.llvm.org/D72227