Page MenuHomePhabricator

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

Authored by MaskRay on Jan 9 2020, 10:04 AM.

Details

Summary

These driver options perform some checking and delegate to MC options -x86-align-branch* and -x86-branches-within-32B-boundaries.

Diff Detail

Event Timeline

MaskRay created this revision.Jan 9 2020, 10:04 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 9 2020, 10:04 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Unit tests: pass. 61683 tests passed, 0 failed and 779 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

@skan Does this look good to you?

MaskRay updated this revision to Diff 237251.Jan 10 2020, 12:06 AM

-malign-branch-boundary= is the primary option. So swap the roles

skan added a comment.Jan 10 2020, 12:21 AM

In D72227 you wrote

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.

This implemetation is consistent with the principles. However, as I mentioned in D72227, I hold a slightly different opinion about the principles.

  1. Different options are position independent. -mA -mB should be the same as -mB -mA.

I think only when A and B are not related, (-mA -mB) == (-mB -mA).

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

Totally agree.

  1. 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 think general options can aslo override specific options. The last one wins. e.g.
The net effect of -malign-branch=fused -malign-branch-within-32B-boundaries should be -malign-branch-boundary=32 -malign-branch=fused+jcc+jmp -malign-branch-prefix-size=5

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

AlignBranch = StringRef() seems unnecessary.

2043

Any reason for precluding 16?

2056

AlignBranch = StringRef() seems unnecessary.

Unit tests: pass. 61743 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 237253.Jan 10 2020, 12:43 AM
MaskRay marked 5 inline comments as done.

Delete a AlignBranch = StringRef()

skan added a comment.Jan 10 2020, 12:46 AM

Add revison D72225 as parent?

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

Any reason for precluding 16?

MaskRay added inline comments.Jan 10 2020, 12:49 AM
clang/lib/Driver/ToolChains/Clang.cpp
2043

I did not preclude 16.

2056

This comment was for the previous diff.

Unit tests: fail. 61741 tests passed, 2 failed and 780 were skipped.

failed: Clang.Driver/x86-malign-branch.c
failed: Clang.Driver/x86-malign-branch.s

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay marked an inline comment as done.Jan 10 2020, 12:59 AM

The commutative property (independence of options) makes option composition easier. clangDriver makes heavy use of getLastArg and hasArg. Without the commutative property, it would now be able to shuffle code around.

The -malign-branch=jmp -mno-branches-within-32B-boundaries issue does not matter that much. I do not expect -mno-branches-within-32B-boundaries to be used much. If we want to disable branch alignment, -malign-branch-boundary=0 can be used.

Add revison D72225 as parent?

-x86-align-branch-prefix-size= does not work without D72225, but I think the two patches can be independent. GCC does not support these options and no projects rely on the option yet.

skan added inline comments.Jan 10 2020, 2:44 AM
clang/lib/Driver/ToolChains/Clang.cpp
2043

I am sorry that I misunderstood the word "preclude". As far as I know, we would like AlignBranchBoundary>=32

skan added a comment.Jan 10 2020, 3:04 AM

The commutative property (independence of options) makes option composition easier. clangDriver makes heavy use of getLastArg and hasArg. Without the commutative property, it would now be able to shuffle code around.

The -malign-branch=jmp -mno-branches-within-32B-boundaries issue does not matter that much. I do not expect -mno-branches-within-32B-boundaries to be used much. If we want to disable branch alignment, -malign-branch-boundary=0 can be used.

The behaviour of your proposal is "specific option is always override the gerneral option, no matter which is the last". I still prefer not to support the override, which makes things more clear. But I am fine if other reviewers believe this behaviour is reasonable. @jyknight @reames @craig.topper

clang/include/clang/Basic/DiagnosticDriverKinds.td
254

The error information "must be one of: " is kind of misleading.
It seems that one kind of branch can be aligned.

skan added inline comments.Jan 10 2020, 3:06 AM
clang/include/clang/Basic/DiagnosticDriverKinds.td
254

The error information "must be one of: " is kind of misleading.
It seems that "only" one kind of branch can be aligned.

MaskRay updated this revision to Diff 237476.EditedJan 10 2020, 7:26 PM
MaskRay edited the summary of this revision. (Show Details)

Delete -mno-branches-within-32B-boundaries (GNU as does not support it)
Use comma joined list instead of +. CommaJoined is more conventional. For example, -mrecip= and -fsanitize= a comma separated list.

MaskRay marked 5 inline comments as done.Jan 10 2020, 7:28 PM
MaskRay added inline comments.
clang/include/clang/Basic/DiagnosticDriverKinds.td
254

Added each element

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

GNU as allows -malign-branch-boundary=16. I think we should allow it as well. It may not be optimal, but it does not hurt to give the user more choices.

Unit tests: pass. 61771 tests passed, 0 failed and 780 were skipped.

clang-tidy: fail. Please fix clang-tidy findings.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

MaskRay updated this revision to Diff 238120.Jan 14 2020, 3:15 PM
MaskRay marked 2 inline comments as done.

Make driver a thin wrapper since we are going to add a master option in MC (D72738)

Unit tests: fail. 61862 tests passed, 1 failed and 781 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_sharedtimedmutex_requirements/thread_sharedtimedmutex_class/try_lock.pass.cpp

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

skan accepted this revision.Jan 14 2020, 9:44 PM

LGTM
The principle of override here is consistent with MC (D72738). So I think this patch is better than D72227.

This revision is now accepted and ready to land.Jan 14 2020, 9:44 PM
This revision was automatically updated to reflect the committed changes.

@MaskRay Did you merge it to LLVM 10 branch?

@MaskRay Did you merge it to LLVM 10 branch?

It is included in the release branch.

git branch origin/release/10.x --contains 5ca24d09aefaedf8e4148c7fce4b4ab0c4ecc72a # suceeded

MaskRay retitled this revision from [Driver][X86] Add -malign-branch* and -malign-branch-within-32B-boundaries to [Driver][X86] Add -malign-branch* and -mbranches-within-32B-boundaries.Jan 17 2020, 9:27 AM
MaskRay edited the summary of this revision. (Show Details)

@MaskRay Did you merge it to LLVM 10 branch?

It is included in the release branch.

git branch origin/release/10.x --contains 5ca24d09aefaedf8e4148c7fce4b4ab0c4ecc72a # suceeded

Thank you!