Page MenuHomePhabricator

[Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries
AbandonedPublic

Authored by skan on Jan 5 2020, 5:41 AM.

Details

Summary

Add -mbranches-within-32B-boundaries to align branches within a 32-Byte
boundary to reduce the potential performance loss of the microcode
update. It is equivalent to the combination of three options:

-malign-branch=fused,jcc,jmp
-malign-branch-boundary=32
-malign-branch-prefix-size=5

-malign-branch* and -mbranches-within-32B-boundaries override each
other, the last one wins. e.g

The net effect of -mbranches-within-32B-boundaries -malign-branch=jcc
is:
-malign-branch=jcc
-malign-branch-boundary=32
-malign-branch-prefix-size=5

The net effect of -malign-branch=jcc -mbranches-within-32B-boundaries
is:
-malign-branch=fused+jcc+jmp
-malign-branch-boundary=32
-malign-branch-prefix-size=5

Diff Detail

Event Timeline

skan created this revision.Jan 5 2020, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 5 2020, 5:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
skan updated this revision to Diff 236280.Jan 5 2020, 6:26 PM
MaskRay added inline comments.Jan 5 2020, 11:54 PM
clang/lib/Driver/ToolChains/Clang.cpp
2015

The convention is camelCase. This file is inconsistent but some newly added functions follow this convention.

2017
  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.

2050

OPT_mbranches_within_32B_boundaries should provide default values which can be overridden by more specific options.

clang/test/Driver/intel-align-branch.c
28 ↗(On Diff #236280)

Since you have a -mno.. -m.. test, the -m.. test is redundant.

35 ↗(On Diff #236280)

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)

skan updated this revision to Diff 236329.Jan 6 2020, 5:12 AM
skan marked 5 inline comments as done.
skan added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2017

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]

2050

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 :-)

clang/test/Driver/intel-align-branch.c
28 ↗(On Diff #236280)

Done

35 ↗(On Diff #236280)

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?

annita.zhang requested changes to this revision.Jan 6 2020, 9:53 PM

A general comment. All the tests have been done with -malign-branch-prefix-size=5. I don't see any explicit reason to change the default prefix size from 5 to 4.

This revision now requires changes to proceed.Jan 6 2020, 9:53 PM
annita.zhang added inline comments.Jan 6 2020, 10:54 PM
clang/include/clang/Driver/Options.td
2210

"aligned to prevent from being across or against the boundary of specified size."

2218

"types). The branches' types are combination of jcc, fused, "

2220–2223

"jcc indicates conditional jumps, fused indicates fused conditional jumps,"
"jmp indicates unconditional jumps, call indicates direct and indirect calls,"
"ret indicates rets, indirect indicates indirect jumps."

2229

Is there an upbound for this parameter?

2239

-malign-branch-prefix-size=5.">;

annita.zhang added inline comments.Jan 6 2020, 10:56 PM
clang/lib/Driver/ToolChains/Clang.cpp
2057

CmdArgs.push_back(Args.MakeArgString("-x86-align-branch-prefix-size=5"));

skan updated this revision to Diff 236547.Jan 7 2020, 3:41 AM
skan marked 5 inline comments as done.
skan added inline comments.
clang/include/clang/Driver/Options.td
2229

yes, both upbound and default value is 5

skan edited the summary of this revision. (Show Details)Jan 7 2020, 8:30 PM
MaskRay added inline comments.Jan 8 2020, 12:12 PM
clang/include/clang/Driver/Options.td
2212

-x86-align-branch-boundary=0

We should not expose llc internal option names.

clang/lib/Driver/ToolChains/Clang.cpp
2050

-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

clang/test/Driver/intel-align-branch.c
35 ↗(On Diff #236280)

You need a test on a different architecture to check that the warning is emitted as intended.

skan updated this revision to Diff 236945.Jan 8 2020, 6:14 PM
skan marked an inline comment as done.
skan marked an inline comment as done.
MaskRay added inline comments.Jan 9 2020, 12:22 AM
clang/include/clang/Driver/Options.td
2205

Group<m_x86_Features_Group>

2209

"It should be 0 or a power of 2 no less than 16."

Do you want to preclude 16?

Long descriptions should be placed in DocBrief values.

2212

"The default value 0 does not align branches."

2243

HelpText<"Opposite to -mbranches-within-32B-boundaries.">;

Delete. No need to elaborate.

skan updated this revision to Diff 237019.Jan 9 2020, 4:50 AM
skan marked 5 inline comments as done.
craig.topper added inline comments.Jan 9 2020, 8:24 AM
clang/include/clang/Driver/Options.td
2205

Doesn't m_x86_Features_Group make things get collected for -target-features? I don't think this should be part of that.

Created D72463 (-mbranches-within-32B-boundaries can be overridden by -malign-branch*; added test coverage)

LuoYuanke added inline comments.Jan 9 2020, 7:19 PM
clang/lib/Driver/ToolChains/Clang.cpp
2050

I have no preference. What's the general rule for such case in LLVM? Is there any similar option design before?

MaskRay added inline comments.Jan 9 2020, 8:07 PM
clang/lib/Driver/ToolChains/Clang.cpp
2050

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).

skan updated this revision to Diff 237245.Jan 9 2020, 10:59 PM
skan marked an inline comment as done.
skan added inline comments.
clang/include/clang/Driver/Options.td
2209

No, I don't want to preclude 16.

skan marked an inline comment as done and an inline comment as not done.Jan 9 2020, 11:09 PM
skan added inline comments.
clang/lib/Driver/ToolChains/Clang.cpp
2050

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.

skan updated this revision to Diff 237491.Jan 11 2020, 4:54 AM
skan retitled this revision from Add options for clang to align branches within 32B boundary to [Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries.
skan edited the summary of this revision. (Show Details)
skan abandoned this revision.Jan 14 2020, 9:48 PM

The override principle of driver should keep same with MC (D72738). D72463 is better, so I prefer to abandon this revision.