skan marked an inline comment as done and an inline comment as not done.
skan 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)) {
----------------
MaskRay wrote:
> 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).
> 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.
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
