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

[Kan Shengchen via Phabricator via llvm-commits]

skan marked 9 inline comments as done.
skan added inline comments.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:531
+        return;
+      PF->setAlignment(AlignBoundary);
+      PF->setEmitNops(true);
----------------
MaskRay wrote:
> You can remove `PF->setAlignment(AlignBoundary);` here.
 The `MCBoundaryAlignFragment` may not emit anything, so the constructor of  `MCBoundaryAlignFragment` doesn't set the `AlignBoundary` with value provided by the user. The data member `AlignBoundary` will be not corretly set here if `PF->setAlignment(AlignBoundary)` is removed.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:537
+      // instruction.
+      PF->setAlignment(AlignBoundary);
+      PF->setMaxBytesToEmit(GetRemainingPrefixSize(PrevInst));
----------------
MaskRay wrote:
> And here.
I think we can not remove it.


================
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)) {
----------------
MaskRay wrote:
> If you place `F->setAlignment(AlignBoundary)` here, you can avoid 2 setAlignment calls in `X86AsmBackend::alignBranchesBegin`.
If a `MCBoundaryAlignFragment` is not used to emit anything, I prefer it doesn't set the alignment.  If we place `F->setAlignment(AlignBoundary)` here, the operation `setAlignment(AlignBoundary)` is unnecessary for those `MCBoundaryAlignFragments` that will not emit anything. 


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:582
+
+  PrevInst = Inst;
+
----------------
MaskRay wrote:
> It seems unnecessary to move it from alignBranchesBegin here.
The purpose of moving `PrevInst = Inst` here is to make the early return in `alignBranchesBegin` simple. Otherwise, we need to write `PrevInst = Inst; return` in alignBranchesBegin` rather than `return`.


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

https://reviews.llvm.org/D72225