MaskRay added inline comments.
================
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)) {
----------------
LuoYuanke wrote:
> MaskRay wrote:
> > 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
> I have no preference. What's the general rule for such case in LLVM? Is there
> any similar option design before?
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).
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
