This is an archive of the discontinued LLVM Phabricator instance.

[BranchAlign] Add master --x86-branches-within-32B-boundaries flag
ClosedPublic

Authored by reames on Jan 14 2020, 2:52 PM.

Details

Summary

This flag was originally part of D70157, but was removed as we carved away pieces of the review. Since we have the nop support checked in, and it appears mature(*), I think it's time to add the master flag. For now, it will default to nop padding, but once the prefix padding support lands, we'll update the defaults.

  • I can now confirm that downstream testing of the changes which have landed to date - nop padding and compiler support for suppressions - is passing all of the functional testing we've thrown at it. There might still be something lurking, but we've gotten enough coverage to be confident of the basic approach.

Note that the new flag can be used either when assembling an .s file, or when using the integrated assembler directly from the compiler. The later will use all of the suppression mechanism and should always generate correct code. We don't yet have assembly syntax for the suppressions, so passing this directly to the assembler w/a raw .s file may result in broken code. Use at your own risk.

Also note that this isn't the wiring for the clang option. I think the most recent review for that is D72227, but I've lost track, so that might be off.

Diff Detail

Event Timeline

reames created this revision.Jan 14 2020, 2:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2020, 2:52 PM
MaskRay added inline comments.Jan 14 2020, 2:59 PM
llvm/test/MC/X86/align-branch-64-1a.s
7

Compare the llvm-mc output with the previous one.

cmp %t1 %t2 returns 0 => identical output.

I think the most recent review for that is D72463. D72227 just copied what I did in D72463.

reames updated this revision to Diff 238122.Jan 14 2020, 3:27 PM

Address review comment

reames marked an inline comment as done.Jan 14 2020, 3:27 PM
MaskRay added inline comments.Jan 14 2020, 4:10 PM
llvm/test/MC/X86/align-branch-64-1a.s
7
llvm-mc -filetype=obj -triple x86_64 --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp %p/Inputs/align-branch-64-1.s -o %t1
llvm-objdump -d %t1 | FileCheck %s

llvm-mc ... -o %t2
cmp %t1 %t2

Compare the object files, not the llvm-objdump output.

reames updated this revision to Diff 238133.Jan 14 2020, 4:15 PM

Address review comment

reames marked an inline comment as done.Jan 14 2020, 4:15 PM
MaskRay accepted this revision.Jan 14 2020, 5:37 PM
MaskRay added inline comments.
llvm/test/MC/X86/align-branch-64-1a.s
2

llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp %p/Inputs/align-branch-64-1.s -o %t1 should be placed here.

Add llvm-objdump -d %t1 | FileCheck %s

5

And delete this line.

This revision is now accepted and ready to land.Jan 14 2020, 5:37 PM
skan added a comment.Jan 14 2020, 6:16 PM

I think the most recent review for that is D72463. D72227 just copied what I did in D72463.

Just to clarify that D72227 was opened first, then D72463 was opened later and changed the principle of option override on the basis of D72227's code and made some refinements. After that, some refinements were migrated back to D72227. D72227 and D72463 always adhere to different override principle. It is inappropriate to say who copied it.

skan accepted this revision.Jan 14 2020, 6:16 PM

LGTM

This revision was automatically updated to reflect the committed changes.