This is an archive of the discontinued LLVM Phabricator instance.

[X86][MC] Fix the bug for prefix padding support
ClosedPublic

Authored by skan on Mar 17 2020, 7:44 AM.

Details

Summary

There is a tiny logic error of D75300, making branch is not
correctly aligned with option -x86-pad-max-prefix-size

Diff Detail

Event Timeline

skan created this revision.Mar 17 2020, 7:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2020, 7:45 AM
skan updated this revision to Diff 250964.Mar 17 2020, 7:20 PM

Delete the unimplemented check in test file (we haven't supported relaxing
the branches to be aligned to reduce the bytes of nops)

reames requested changes to this revision.Mar 18 2020, 9:16 AM

It's not clear what bug you think you're fixing. Please provide a description of the bad behaviour.

llvm/test/MC/X86/align-branch-64-basic.s
106

Please restore this test and show any diff. This test is not an unimplemented case, it's testing the jne being relaxed which is happening.

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s
10

This case should already be handled by skipping instructions past the branch align.

This revision now requires changes to proceed.Mar 18 2020, 9:16 AM
skan marked 2 inline comments as done.Mar 18 2020, 6:46 PM
skan added inline comments.
llvm/test/MC/X86/align-branch-64-basic.s
106

Why do you think we can relax this jne to reduce nops? This jne is after boundaryalign.

llvm/test/MC/X86/align-branch-64-pad-max-prefix.s
10

No, it's not handled. Your can run this test case without this patch to prove.

LuoYuanke added inline comments.Mar 19 2020, 10:13 PM
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s
10

Do you mean when jmp size is expanded, it cross the align boundary? That seems a problem. The jmp size should be frozen after nop padding.

skan marked an inline comment as done.Mar 19 2020, 11:52 PM
skan added inline comments.
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s
10

Yes, the jump's size is expanded and leans against the boundary. This patch is trying to fix this bug.

skan marked an inline comment as done.Mar 24 2020, 10:14 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
973–977

The existing code in finishLayout intends to not pad instructions following boundary align by checking whether F is MCBoundaryAlignFragment. But this early continue prevents the control flow from reaching that check. This patch is pretty small (delete 3 lines) and is a bug fix, could reviewers take a close look at it?

reames accepted this revision.Mar 26 2020, 8:49 PM

LGTM, thank you for the explanation from Tuesday, sorry for not noticing that sooner.

This revision is now accepted and ready to land.Mar 26 2020, 8:49 PM
This revision was automatically updated to reflect the committed changes.
MaskRay added inline comments.Mar 26 2020, 11:23 PM
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s
2

I am late, but the indentation is really not needed.

-pc-linux-gnu is really not needed.

15

Add -CHECK: if applicable.

skan marked 2 inline comments as done.Mar 27 2020, 12:43 AM
skan added inline comments.
llvm/test/MC/X86/align-branch-64-pad-max-prefix.s
2

Almost all test cases align-branch* use the indentation , -pc-linux-gnu , we can delete them all at once later if necessary.

15

sorry, didn't get your point.