[PATCH] D72225: Align branches within 32-Byte boundary(Prefix padding)

[Kan Shengchen via Phabricator via llvm-commits]

skan marked 4 inline comments as done.
skan added a comment.

In D72225#1816298 <https://reviews.llvm.org/D72225#1816298>, @MaskRay wrote:

> The prefix insertion logic is scattered (alignBranchesBegin, alignBranchesEnd, and relaxBoundaryAlign), though they are tightly coupled and share a fair amount of information. I am thinking whether placing the logic in one place will remove some abstraction and make the overall logic simpler to understand.
>
> `MCBoundaryAlignFragment` is currently relaxed in the loop as `MCRelaxableFragment`. `MCBoundaryAlignFragment`s are pre-allocated.
>
> If we don't require `MCBoundaryAlignFragment` to be processed in the same loop as other fragments. We can (1) remove pre-allocation of `MCBoundaryAlignFragment` (2) layout everything (except `MCBoundaryAlignFragment`) (3) loop over fragments and compute the candidate placement points of `MCBoundaryAlignFragment`. When we find a jcc which requires padding, pick the candidates on its left and place padding. The candidates can be maintained as a slide window. Whenever we encounter a MCAlignFragment, clear the sliding window. After handling a jcc which requires padding, clear the sliding window as well.


Although the data structure used to hold MCFragment is doubly linked list, which means we can insert a fragment anywhere, the `MCObjectStreamer` only has the interface `insert()` to insert a fragment at the end of the list.  It may be hard to add an interface for `MCObjectStreamer` to insert fragment anywhere safely, that's the reason why I preallocate the `MCBoundaryAlignFragment`.

If you have interest in designing such an interface for  `MCObjectStreamer` and placing the logic in one place, you can reimplement the Prefix Padding after the patch is landed, I will be very glad to help you review that patch.


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

https://reviews.llvm.org/D72225