Page MenuHomePhabricator

[X86] Disable nop padding before instruction following hardcode
ClosedPublic

Authored by skan on Mar 14 2020, 8:18 AM.

Diff Detail

Event Timeline

skan created this revision.Mar 14 2020, 8:18 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 14 2020, 8:18 AM
skan updated this revision to Diff 250386.Mar 14 2020, 2:19 PM

Avoid the warning reported by clang-tidy

LuoYuanke added inline comments.Mar 15 2020, 7:42 PM
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
507

Does hard coded instruction occupy one separate fragment? Need comments to explain how to check hard code. Better to have a diagram.

skan marked an inline comment as done.Mar 15 2020, 8:35 PM
skan added inline comments.
llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp
507

Hard coded instruction may occupy one separate fragment. It's not a good way to enumerate all the cases but I will add more comments here.

skan updated this revision to Diff 250455.Mar 15 2020, 8:49 PM

Address review comments

skan updated this revision to Diff 250468.Mar 15 2020, 9:55 PM

Update comment

This revision is now accepted and ready to land.Mar 16 2020, 12:37 AM
This revision was automatically updated to reflect the committed changes.

FYI, this patch (and the previous prefix handling) is incomplete. For both of them, you need to prevent not just nop insertion, but also prefix insertion. I don't think this is really a problem for the compiler use case (as we already have the suppression mechanism), but since you seem set on supporting arbitrary assembly I wanted to point that out. I still think that is a bad idea and that you're better off defining syntax for an opt in system.

We probably need some form of a marker on the fragment or a separate fragment to mark a boundary it is unsafe to pad.

skan added a comment.Mar 18 2020, 8:26 PM

FYI, this patch (and the previous prefix handling) is incomplete. For both of them, you need to prevent not just nop insertion, but also prefix insertion.

Yes, this patch doesn't prevent prefix insertion since it does not make more instructions prefixable and this patch does exactly what it describes, namely, "Disable nop padding before ...". D76286 makes more instruction prefixable (be emitted into RelaxableFragment), so I also prevent prefix insertion before the new "relaxable" instruction if it follows prefix or hardcode. As to the old relaxable instruction (relative jump), I thought you believed inserting prefixes before it is not a problem even if it follows prefix or hardcode, since you didn't consider that in D75300.
But according to your comment here, I think I misunderstood you :-( .

We probably need some form of a marker on the fragment or a separate fragment to mark a boundary it is unsafe to pad.

I think you mean "mark a branch" rather than "mark a boundary". I agree we can add a marker to the relaxable fragment to do this. Do you want to write this patch by yourself ? If you do not, I am happy to write it.

I don't think this is really a problem for the compiler use case (as we already have the suppression mechanism), but since you seem set on supporting arbitrary assembly I wanted to point that out.

Yes, we want to support arbitrary assembly since we do have that need.

I still think that is a bad idea and that you're better off defining syntax for an opt in system.

To definie such a syntax, we need to keep consistent with gas, which may take more time. I think we can prevent nop/prefix insertion after hardcode/prefix first, and after the syntax is determined, we can remove the code for checking hardcode/prefix if necessary.

skan added a comment.Mar 19 2020, 8:50 PM

FYI, this patch (and the previous prefix handling) is incomplete. For both of them, you need to prevent not just nop insertion, but also prefix insertion.

Complete solution here D76475.