[PATCH] D70157: Align branches within 32-Byte boundary

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Haven't looked into too many details yet but made some suggestions anyway...



================
Comment at: llvm/include/llvm/MC/MCFragment.h:634
+
+  uint64_t getBoundarySize() const {
+    return AlignBoundarySize;
----------------
Store AlignBoundarySize as a shift value, then `needPadding` doesn't even need to call Log2_64().


================
Comment at: llvm/lib/MC/MCFragment.cpp:426
+  case MCFragment::FT_MachineDependent: {
+    const MCMachineDependentFragment *MF =
+        cast<MCMachineDependentFragment>(this);
----------------
`const auto *`. The type is obvious according to the right hand side.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:104
+      else if (BranchType == "indirect")
+        addKind(AlignBranchIndirect);
+    }
----------------
An unknown value is just ignored, e.g. `--x86-align-branch=unknown`. I think there should be an error, but I haven't looked into the patch detail to confidently suggest how we should surface this error.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:185
+      AlignMaxPrefixSize = 5;
+    } else {
+      assert((X86AlignBranchBoundary == 0 || (X86AlignBranchBoundary >= 32 &&
----------------
> } else {

Should --x86-branches-within-32B-boundaries overwrite --x86-align-branch-boundary and --x86-align-branch and --x86-align-branch-prefix-size? My feeling is that it just provides a default value if either of the three options is not specified.

If you are going to remove `addKind` calls here, you can delete this member function.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:196
+    }
+    if(AlignBoundarySize != 0) {
+      AlignBoundarySize = Align(AlignBoundarySize).value();
----------------
space after if

No curly braces around simple statements.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:660
+  for (auto *F = CF; F && F != LastBranch; F = F->getPrevNode()) {
+    bool IsMF = F->getKind() == MCFragment::FT_MachineDependent;
+    // If there is a fragment that neither has instructions nor is
----------------
`isa<MCMachineDependentFragment>(F)`



================
Comment at: llvm/lib/Target/X86/X86InstrInfo.td:1020
 def X86_COND_B   : PatLeaf<(i8 2)>;  // alt. COND_C
-def X86_COND_AE  : PatLeaf<(i8 3)>;  // alt. COND_NC
+def X86_COND_AE  : PatLeaf<(i8 3)>;  // alt. COND_NC,COND_NB
 def X86_COND_E   : PatLeaf<(i8 4)>;  // alt. COND_Z
----------------
Unintentional change not part of this patch?


================
Comment at: llvm/test/MC/X86/align-branch-32-1a.s:1
+# Check the macro-fusion table
+# RUN: llvm-mc -filetype=obj -triple i386-unknown-unknown --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc+jmp  --x86-align-branch-prefix-size=5 %s | llvm-objdump -d - | FileCheck %s
----------------
Did an older version include 32-1b.s or 32-1c.s? Now they are missing.


================
Comment at: llvm/test/MC/X86/align-branch-64-1b.s:1
+# Check only fused conditional jumps and conditional jumps are aligned with option --x86-align-branch-boundary=32 --x86-align-branch=fused+jcc  --x86-align-branch-prefix-size=5
+# 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
----------------
Create test/MC/X86/Inputs/align-branch-64-1.s and reference it from 1[a-d].s via %S/Inputs/align-branch-64-1.s


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

https://reviews.llvm.org/D70157