[PATCH] D97982: [MC] Introduce NeverAlign fragment type

[Kan Shengchen via Phabricator via llvm-commits]

skan added a comment.

In D97982#2657470 <https://reviews.llvm.org/D97982#2657470>, @Amir wrote:

> @skan:
>
>> Is the purpose of this patch to avoid cmp ends and jmp starts exactly at 64b alignment? If it's the only purpose, I think we can reuse the BoundaryAlign fragment by adding a function like "needPadding" in MCAssembler.cpp.
>
> Yes, that's the purpose. We've looked at BoundaryAlign and aligned instruction bundles, but failed to come up with a simple solution. How do you envision BoundaryAlign avoiding the given alignment boundary falling between cmp and jmp? NeverAlign inserts a one-byte nop before cmp if it would end at a given boundary.
>
>> Could you also upstream the code that inserts/remove the NeverAlign fragment and some test cases? Usage can help us determine if the design of the fragment is reasonable.
>
> We seek to upstream LLVM parts required by BOLT before pushing BOLT. 
> The usage is shown here: https://github.com/facebookincubator/BOLT/blob/68abc968b706b55585b1b8be315aef5d3bf90b1c/bolt/src/BinaryEmitter.cpp#L445
> We analyze the basic block for macro-op fusion pairs; if such a pair is found, we insert NeverAlign fragment after the first instruction (cmp) with a call `MCStreamer::emitNeverAlignCodeAtEnd()`. That's the only external use of the interfaces we add, which makes it non-trivial to test. That's why I'm seeking to add a way to trigger the insertion of NeverAlign fragment (see comment above).

I think most of the logic should be already there for the "NeverAlign" purpose. My concern about the usage of `MCStreamer::emitNeverAlignCodeAtEnd()` is that it ignores some corner cases( see the check in X86AsmBackend::emitInstructionBegin), which may cause correctness issues. BoundaryAlign fragment is designed for JCC erratum originally,  and the logic whether the macro-fused pair crosses the boundary and the length of nop to be inserted lie in the function `MCAssembler::relaxBoundaryAlign`.  As far as I can see, the interface of `MCStreamer::emitNeverAlignCodeAtEnd()` is unnecessary and simple customization of `relaxBoundaryAlign` can achive your purpose easily (setting the size of BoundaryAlign fragment to 1 if given alignment boundary falling between macro-fused pair).  In addition, reusing the BoundaryAlign can make the test trival and `MC/X86/align-branch*.s` gives some examples.


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

https://reviews.llvm.org/D97982