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

[Kan Shengchen via Phabricator via cfe-commits]

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