Page MenuHomePhabricator

[Driver][X86] Add -mpad-max-prefix-size
ClosedPublic

Authored by skan on Apr 6 2020, 8:49 PM.

Details

Summary

The option -mpad-max-prefix-size performs some checking and delegate to MC option -x86-pad-max-prefix-size. This option is designed for eliminate NOPs when we need to align something by adding redundant prefixes to instructions, e.g. it can be used along with -malign-branch, -malign-branch-boundary to prefix padding branch.

It has similar (but slightly different) effect as GAS's option -malign-branch-prefix-size, e.g. -mpad-max-prefix-size can also elminate NOPs emitted by align directive, so we use a different name here. I remove the option -malign-branch-prefix-size since is unimplemented and not needed. If we need to be compatible with GAS, we can make -malign-branch-prefix-size an alias for this option later.

Diff Detail

Event Timeline

skan created this revision.Apr 6 2020, 8:49 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 6 2020, 8:49 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
skan edited the summary of this revision. (Show Details)Apr 6 2020, 8:53 PM
skan updated this revision to Diff 255583.Apr 6 2020, 9:31 PM

format the patch

skan added a comment.Apr 6 2020, 10:34 PM

The remote build failed due to a bug of Pre-merge checks, which tried to apply a landed patch.

skan updated this revision to Diff 255953.Apr 8 2020, 4:12 AM

Rebase

This revision is now accepted and ready to land.Apr 8 2020, 5:14 AM
MaskRay requested changes to this revision.EditedApr 8 2020, 10:03 AM

It has similar (but slightly different) effect as GAS's option -malign-branch-prefix-size, e.g. -mpad-max-prefix-size can also elminate NOPs emitted by align directive, so we use a different name here. I remove the option -malign-branch-prefix-size since is unimplemented and not needed. If we need to be compatible with GAS, we can make -malign-branch-prefix-size an alias for this option later.

This looks like it requires more thorough discussions, doesn't it? We will also need discussions with the binutils side. We requested a GCC compiler driver option long ago but we do not reach a consensus (https://gcc.gnu.org/legacy-ml/gcc/2020-01/msg00358.html). IMHO the approval was in haste. With all due respect, I think I've seen such hasty approval (without any mentioning why such decisions are justifiable) and hasty commits in this area before several times, so I'll click "Request Changes" just in case of an accidental commit.

This revision now requires changes to proceed.Apr 8 2020, 10:03 AM

This looks like it requires more thorough discussions, doesn't it? We will also need discussions with the binutils side. We requested a GCC compiler driver option long ago but we do not reach a consensus (https://gcc.gnu.org/legacy-ml/gcc/2020-01/msg00358.html). IMHO the approval was in haste. With all due respect, I think I've seen such hasty approval (without any mentioning why such decisions are justifiable) and hasty commits in this area before several times, so I'll click "Request Changes" just in case of an accidental commit.

I accept the patch, because it just expose the LLVM option in clang and nobody reply on the patch. Actually we are waiting for you and Phillip to review it. If you have concern on the option. I'm open for discussion.

skan added a comment.Apr 8 2020, 7:04 PM

This looks like it requires more thorough discussions, doesn't it? We will also need discussions with the binutils side. We requested a GCC compiler driver option long ago but we do not reach a consensus (https://gcc.gnu.org/legacy-ml/gcc/2020-01/msg00358.html). IMHO the approval was in haste. With all due respect, I think I've seen such hasty approval (without any mentioning why such decisions are justifiable) and hasty commits in this area before several times, so I'll click "Request Changes" just in case of an accidental commit.

AFAICS, this patch adds -mpad-max-prefix-size for clang, I mentioned -malign-branch-prefix-size in the summary just to clarify it has different behaviour from GAS's option -malign-branch-prefix-size. This patch has nothing to do with GCC compatibility, so I think the reasons for objection are not sufficient.

skan updated this revision to Diff 256164.Apr 8 2020, 8:09 PM

Remove the limit "max padding prefix size <=5" since MC doesn't
have this limit.

MaskRay accepted this revision.Apr 8 2020, 8:29 PM
MaskRay added inline comments.
clang/include/clang/Driver/Options.td
2190–2191

I think DriverOption is the default and you can omit it.

This revision is now accepted and ready to land.Apr 8 2020, 8:29 PM
This revision was automatically updated to reflect the committed changes.