MaskRay added inline comments.
================
Comment at: clang/include/clang/Driver/Options.td:2200
+ "from being across or against the boundary of specified size. "
+ "-x86-align-branch-boundary=0 doesn't align branches.">;
+def malign_branch_EQ
----------------
> -x86-align-branch-boundary=0
We should not expose llc internal option names.
================
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)) {
----------------
skan wrote:
> 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 :-)
>
>
> -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
================
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"
----------------
skan wrote:
> 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?
>
You need a test on a different architecture to check that the warning is
emitted as intended.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D72227/new/
https://reviews.llvm.org/D72227
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
