[PATCH] D72291: Reimplement BoundaryAlign mechanism (mostly, but not quite NFC)

[Kan Shengchen via Phabricator via llvm-commits]

skan added a comment.

Some meta comments:

Currently, the NOP padding version doesn't check the hardcode so that it has some common issues( not introduced by the reimplementation):

**1. Some unfused pair is padded as if it is fused**

  cmp %rax %rcx
  .byte 0x90
  je .Label0

`.byte 0x90` is not emitted by the function `MCObjectStreamer::EmitInstruction`, so current code can't see `.byte 0x90` and thinks the CMP is fused with JE, however, they are not fused apparently.

**2.  Some expected check will fail**
`MCBoudaryAlignFragment` is used to align one instruction or a fused pair, so it's size is expected to be no more than 30(the maximun size of two instructions). 
When we encounter the case

  cmp %rax %rcx
  .rept 30
  .byte 0x90
  .endr
  je .Label0

current code will emit NOP before the CMP and think the size of the instructions to be aligned is sizeof(CMP) + 30 + sizeof(JE) incorrectly, then the size of padding may be greater than 30.

**3. NOP may be inserted after hardcode**
As @jyknight  says,

>    p4_general_done:
>   	.byte 0xf3
>   	ret
> 
>   Inserting anything between the tacked-on-prefix with .byte, and the rest of the instruction written mnemonically, would be bad.

Although all these case can be correctly addressed by adding assembler directive (your proposal D72303 <https://reviews.llvm.org/D72303>) manually,  they can also be dealt with correctly and naturally
by not appending hardcode to fragment that has an instruction.

**Prefix padding patch**
To support for aligning branch with prefix padding, I have uploaded a new patch D72225 <https://reviews.llvm.org/D72225>. All three issues above have been resolved and my design for `MCBoundaryAlignFragment` is really similar to your reimplementation, do you mind if we directly focus on the prefix padding patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72291