[PATCH] D76176: [X86] Disable nop padding before instruction following hardcode

Wed Mar 18 20:37:00 PDT 2020

skan added a comment.

In D76176#1930168 <https://reviews.llvm.org/D76176#1930168>, @reames wrote:

> 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 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76176/new/

https://reviews.llvm.org/D76176