Page MenuHomePhabricator

[BranchAlign] Compiler support for suppressing branch align

Authored by reames on Jan 6 2020, 1:25 PM.



As discussed heavily in the original review (D70157), there's a need for the compiler to be able to selective suppress padding (either nop or prefix) to respect assumptions about the meaning of labels and instructions in generated code.

Rather than wait for syntax to be finalized - which appears to be a very slow process - this patch focuses on the compiler use case and *only* worries about the integrated assembler. To my knowledge, this covers all cases mentioned to date for clang/JIT support.

For testing purposes, I wired it up so that if the integrated assembler was using autopadding for branch alignment (e.g. enabled at command line) then the textual assembly output would contain a comment for each location where padding was enabled or disabled. This seemed like the least painful choice overall.

Note that the result of this patch effective disables the jcc errata mitigation for many constructs (statepoints, implicit null checks, xray, etc...) which is non ideal. It is at least *correct* and should allow us to enable the mitigation for the compiler. Once that's done, and a few other items are worked through, we probably want to come back to this an explore a bundling based approach instead so that we can pad instructions while keeping labels in the right place.

Diff Detail

Event Timeline

reames created this revision.Jan 6 2020, 1:25 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2020, 1:25 PM
reames updated this revision to Diff 236451.Jan 6 2020, 2:02 PM

Really helps if I upload the patch correctly...

reames planned changes to this revision.Jan 6 2020, 3:52 PM

It looks like the actual padding support got lost in one of the rebases. I'll need to rewrite that part. Though, I think I'm going to take an alternate approach which prioritizes the integrated assembler and simple marks nopadding regions with comments (i.e. rather than directives). That gives me most of what I'm looking for testing wise, and is a lot less likely to be controversial.

reames updated this revision to Diff 236694.Jan 7 2020, 3:03 PM
reames edited the summary of this revision. (Show Details)

Completely rework patch to support integrated assembler only + comments for testing.

(Warning: patch description heavily modified)

MaskRay added inline comments.Jan 7 2020, 3:48 PM

Some targets (XCore) do not have a MCAsmBackend (Target::hasMCAsmBackend() is false).

Assembler->getBackend() cannot be called.

Check test/MC/Disassembler/XCore/xcore.txt

reames marked an inline comment as done.Jan 7 2020, 3:58 PM
reames added inline comments.

Oy, will make the call conditionally.

reames updated this revision to Diff 236719.Jan 7 2020, 4:48 PM
reames marked an inline comment as done.

Generally good, only some suggestions for the tests.


We could also be more specific. As an example, disabling auto padding only when --x86-align-branch=jcc is included & jcc is emitted by the relevant LowerXXXXX. The value is low, so being unconditional should be fine.


Whitespace on an otherwise empty line.

Some other places in this file has such whitespace.


This should be unnecessary.


Make llvm-objdump | FileCheck a separate step.

Add a separate assembly output test, checking #noautopadding is emitted.


Just check the intended offset sequence.

1: callq
6: callq
b: callq
10: callq
15: callq
1a: callq
1f: callq
reames marked 3 inline comments as done.Jan 7 2020, 6:37 PM
reames added inline comments.

Sorry, was this a comment, or a request? I definitely do not want to make this any more complicated at this time. Only reason I bothered filtering the comments at all was to keep the test churn down.


What specifically?


I'd really prefer not to introduce a temporary file here. It adds no value.

Happy to add the textual assembly check.

MaskRay added inline comments.Jan 7 2020, 8:23 PM

Delete ; NOTE: Assertions have been autogenerated by utils/

This is a hand-written test file.


I'd really prefer not to introduce a temporary file here. It adds no value.

OK. No strong opinion here.

skan added inline comments.Jan 7 2020, 11:18 PM

NoAutoPaddingScope(MCStreamer &OS) : OS(OS) {

I recommend running command like git diff -U0 --no-color HEAD^ | -i -p1 to format the patch.


keep the code in one line


Remove blank here

skan added inline comments.Jan 8 2020, 12:50 AM

The OldAllowAutoPadding is immutable indeed. I recommend declaring is as a const member and intializing it the initialization list.
Also, have your considered using the name DefaultAllowAutoPadding?

reames updated this revision to Diff 236847.Jan 8 2020, 9:36 AM
reames marked 2 inline comments as done.
reames updated this revision to Diff 236848.Jan 8 2020, 9:40 AM
reames marked 5 inline comments as done.

Missed a review comment, now addressed.

reames added a comment.Jan 8 2020, 9:41 AM

I've addressed the vast majority of style comments and have noted why I don't think the remainder are appropriate.


This is not idiomatic for non-accessors.


On further reflection, I don't plan to add the textual assembly either. That exact case is already covered in the other test file.

MaskRay accepted this revision.Jan 8 2020, 9:46 AM
MaskRay added inline comments.

CHECK-NEXT: will be better.

This revision is now accepted and ready to land.Jan 8 2020, 9:46 AM
This revision was automatically updated to reflect the committed changes.