Page MenuHomePhabricator

[X86] Reduce the number of emitted fragments due to branch align
ClosedPublic

Authored by skan on Mar 2 2020, 1:07 AM.

Details

Summary

Currently, a BoundaryAlign fragment may be inserted after the branch
that needs to be aligned to truncate the current fragment, this fragment is
unused at most of time. To avoid that, we can insert a new empty Data
fragment instead. Non-relaxable instruction is usually emitted into Data
fragment, so the inserted empty Data fragment will be reused at a high
possibility.

Diff Detail

Event Timeline

skan created this revision.Mar 2 2020, 1:07 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 2 2020, 1:07 AM
skan planned changes to this revision.Mar 2 2020, 4:17 AM

Found some internal fail tests

skan updated this revision to Diff 247671.Mar 2 2020, 9:04 AM

Inserting a fragment before the current insertion point is problematic now,
avoid doing this

skan edited the summary of this revision. (Show Details)Mar 2 2020, 9:05 AM
reames requested changes to this revision.Mar 2 2020, 12:43 PM
reames added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
561

I believe you can simply insert a new empty data fragment instead of adding the special state to all data fragments.

Also, you only need to do so if the last fragment was a data fragment.

This revision now requires changes to proceed.Mar 2 2020, 12:43 PM
skan marked an inline comment as done.Mar 2 2020, 5:37 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
561

If the next intruction is emitted into a relaxable fragment, the empty data fragment will be unused, is it acceptable to you?

skan updated this revision to Diff 247812.Mar 3 2020, 12:31 AM

Rebase and address the review comment

skan retitled this revision from [BranchAlign] Reduce the number of emitted fragments due to branch align to [X86] Reduce the number of emitted fragments due to branch align.Mar 3 2020, 12:36 AM
skan edited the summary of this revision. (Show Details)
skan updated this revision to Diff 247815.Mar 3 2020, 12:57 AM

Remove unused temp variable

skan updated this revision to Diff 247834.Mar 3 2020, 2:34 AM

Not invalidFragment when we relax BoundaryAlign

reames requested changes to this revision.Mar 3 2020, 10:39 AM
reames added inline comments.
llvm/lib/MC/MCAssembler.cpp
1022

Do me a favour and leave this line in for this patch. It can be removed in a follow up, but the assumptions about offset calculation caching are subtle and it's clearly conservatively correct to invalidate too much. :)

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
513

Does this need to be a runtime check? Or can it be an assert?

"assert(OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign)" to be clear.

Also, add braces. Even though there's a single statement the block comment deserves clear demarcation.

518

Add to comment: and we'll tie them together in branchBoundaryEnd

535–541

Drop the else since the previous if returned.

554

Might be good to add an assert here about the expected instructions between pending BA and this one.

559

I believe we only need this bit of code if we tied the aligned instruction into a previous BA. (i.e. we can early return above if there's no pending BA.)

561

Yes. This is an idiom we already use in a few places and we can fix them all at once if it becomes an issue.

This revision now requires changes to proceed.Mar 3 2020, 10:39 AM
skan marked an inline comment as done.Mar 3 2020, 8:50 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
513

yes, we need to check it at runtime. An AlignFragment might be inserted after previous instruction(see the comment below), we need to insert a new BoundaryAlign before the JCC if there is a AlignFragment between the fusible pair.

skan marked an inline comment as done.Mar 3 2020, 9:12 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
554

How to check the expected instructions here?

skan updated this revision to Diff 248100.Mar 3 2020, 9:27 PM

Address the review comment

skan marked 4 inline comments as done.Mar 3 2020, 9:29 PM
skan marked an inline comment as done.Mar 3 2020, 9:54 PM
skan added inline comments.
llvm/test/MC/X86/align-branch-64-negative.s
26–27

Can I remove the second test?

skan updated this revision to Diff 248365.Mar 4 2020, 6:12 PM

Rebase

skan added a comment.Mar 5 2020, 5:31 PM

Ping. There are no new comments for two days.

LuoYuanke added inline comments.Mar 9 2020, 8:43 PM
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
508

PendingBoundaryAlign has been set to nullptr in alignBranchesEnd()?

551

Only check PendingBoundaryAlign is enough? In alignBranchesBegin(), if needAlignInst(Inst), a PendingBoundaryAlign is added.

556

After setting PendingBoundaryAlign to nullptr, it seems there is no chance to reset LastFragment of PendingBoundaryAlign for macro fusion case.

558

I don't understand why further data isn't added to the current data. Is it because we need get instruction size? Could you comments it in the code?

skan marked 4 inline comments as done.Mar 9 2020, 9:33 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
508

We need to clear the PendingBoundaryAlign

  • If macro fusion doesn't happen.
  • Or if the PendingBoundaryAlign is assigned a fragment

alignBranchesEnd is responsible for the second case and alignBranchesBegin is responsible for the first case.

551

That' not enough. We need to handle the macro fusion case. When PrevInst is the first instruction in a fusible pair, the PendingBoundaryAlign is not null and we should return directly here.

556

The macro fusion case is corretly handled here. We do not need to reset LastFragment of PendingBoundaryAlign. Only when the Inst needs to be aligned, we can reach here and LastFragment is only set once.

558

Yes, it is because we need get instruction size. I will add it to comments.

skan updated this revision to Diff 249273.Mar 9 2020, 9:47 PM

Add comments for why we need to insert a new DataFragment

LuoYuanke accepted this revision.Mar 9 2020, 10:53 PM

LGTM. Better to hold for few days to have Philips and MaskRay review as well.

skan added a comment.Mar 10 2020, 8:31 PM

Ping. I applied this patch and passed the chek-all tests and found no new fails when running SPEC. Does this patch look good to you ?@reames @MaskRay

MaskRay added inline comments.Mar 10 2020, 9:39 PM
llvm/test/MC/X86/align-branch-64-negative.s
26–27

Why is the test deleted?

skan marked an inline comment as done.Mar 10 2020, 10:51 PM
skan added inline comments.
llvm/test/MC/X86/align-branch-64-negative.s
26–27

This test is a negative test and we expect the label is bounded to the end of the call rather than the start of the jmp. As commented in this test file, the label "label_after" is in the right place now because we insert a new empty DataFragment after the call.

reames requested changes to this revision.Mar 11 2020, 9:29 PM
reames added inline comments.
llvm/test/MC/X86/align-branch-64-negative.s
26–27

Please update the check line instead. I can't tell from the diff or not whether the change made is correct or not.

p.s. Commenting out a test is almost never the right answer.

This revision now requires changes to proceed.Mar 11 2020, 9:29 PM
reames accepted this revision.Mar 11 2020, 9:41 PM

I applied the patch and reviewed the changed code sequence directly. It's more correct than what was there before.

LGTM, but please make sure the test check lines are updated so that the diff is visible before landing.

This revision is now accepted and ready to land.Mar 11 2020, 9:41 PM
skan updated this revision to Diff 249840.Mar 11 2020, 10:53 PM

Address review comments

This revision was automatically updated to reflect the committed changes.