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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:535
+  /// of using the provided value.
+  bool EmitNops = false;
+  /// Value to use for filling padding bytes if existing.
----------------
You can keep `EmitNops` above. The first 2 bytes of `MCBoundaryAlignFragment` and the tail of `MCFragment` shared the same word.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:548
+
+  /// \name Accessors
+  /// @{
----------------
Drop `\name Accessors`. It is not useful.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:564
+
+  bool hasEmit() const { return EmitNops || Value.hasValue(); }
+
----------------
I find this method confusing. Updating the call sites to use `hasEmitNops() || hasValue()`


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1005
+  uint64_t AlignedOffset = Layout.getFragmentOffset(TF);
+  for (auto *F = TF->getPrevNode(); !isa<MCBoundaryAlignFragment>(F);
+       F = F->getPrevNode()) {
----------------
Is it guaranteed `F->getPrevNode()` will not be executed on the first Fragment?


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1009
+    AlignedSize += Size;
+    AlignedOffset -= Size;
   }
----------------
`AlignedOffset` can be defined after AlignedSize is computed.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1014
+  // [BF,BF.getFragment).
+  uint64_t FixedValue = [&]() {
+    uint64_t N = 0;
----------------
FixedValue doesn't have to be an immediately invoked function expression.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1028
+  if (!BF.hasEmitNops()) {
+    assert((*(BF.getNextNode())).hasInstructions() &&
+           "The fragment doesn't have any instruction.");
----------------
`*(xxx)` -> `xxx->`


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1038
+  uint64_t OldSize = BF.getSize();
   if (NewSize == OldSize)
     return false;
----------------
`if (NewSize == BF.getSize()`


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:166
     AlignBranchType = X86AlignBranchKindLoc;
+    AlignMaxPrefixSize = std::min<uint8_t>(X86AlignBranchPrefixSize, 5);
   }
----------------
`AlignMaxPrefixSize = X86AlignBranchPrefixSize;`

The error checking and normalization (to 5) should be done in an earlier place.


================
Comment at: llvm/test/MC/X86/align-branch-64-2d.s:1
+# Check only indirect jumps and calls are aligned with option --x86-align-branch-boundary=32 --x86-align-branch=indirect+call --x86-align-branch-prefix-size=4
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=indirect+call --x86-align-branch-prefix-size=4 %p/Inputs/align-branch-64-2.s  | llvm-objdump -d  - | FileCheck %s
----------------
When writing tests, make sure llvm-mc and GNU as emit jmp of the same length. There are differences (D72197).


================
Comment at: llvm/test/MC/X86/align-branch-64-8a.s:1
+# Check the case multiple CMPs are followed a jcc is correctly handled.
+# RUN: llvm-mc -filetype=obj -triple x86_64-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc  --x86-align-branch-prefix-size=5 %s | llvm-objdump -d  - | FileCheck %s
----------------
Use `## ` for comments.

`x86_64-unknown-unknown` can be simplified to `x86_64` (the default is ELF).


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

https://reviews.llvm.org/D72225