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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added a comment.

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.



================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:531
+        return;
+      PF->setAlignment(AlignBoundary);
+      PF->setEmitNops(true);
----------------
You can remove `PF->setAlignment(AlignBoundary);` here.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:537
+      // instruction.
+      PF->setAlignment(AlignBoundary);
+      PF->setMaxBytesToEmit(GetRemainingPrefixSize(PrevInst));
----------------
And here.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:565
+    // instruction, so a placeholder is put here if necessary.
+    OS.getOrCreateBoundaryAlignFragment();
+  } else if (shouldAddPrefix(Inst)) {
----------------
If you place `F->setAlignment(AlignBoundary)` here, you can avoid 2 setAlignment calls in `X86AsmBackend::alignBranchesBegin`.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:582
+
+  PrevInst = Inst;
+
----------------
It seems unnecessary to move it from alignBranchesBegin here.


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

https://reviews.llvm.org/D72225