[PATCH] D101817: [MC][X86] Automatic alignment for Macro-Op Fusion

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/include/llvm/MC/MCFragment.h:563-567
 /// Represents required padding such that a particular other set of fragments
 /// does not cross a particular power-of-two boundary. The other fragments must
 /// follow this one within the same section.
+/// If AvoidEndAlign is set, this fragment will emit a minimum size nop to
+/// prevent the fragment following it from ending at a given \p AlignBoundary.
----------------
The comments here is weired after you added the new usage to the fragment, you need to refine it.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1086-1087
   uint64_t AlignedSize = 0;
-  for (const MCFragment *F = BF.getLastFragment(); F != &BF;
-       F = F->getPrevNode())
-    AlignedSize += computeFragmentSize(Layout, *F);
-
+  uint64_t NewSize = 0U;
   Align BoundaryAlignment = BF.getAlignment();
+
----------------
`uint64_t NewSize = 0` is enough, we used U suffix before because of the conditional operator.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:80-81
+    "x86-align-for-macrofusion", cl::init(false),
+    cl::desc("Align cmp+jcc pairs to avoid the 64B boundary falling between "
+             "instructions, improving Macro-Op Fusion. May break assumptions "
+             "about labels corresponding to particular instructions, and "
----------------
The words "cmp+jcc" and "falling between" seem not accurate. 


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:165-172
+    if (X86AlignForMacroFusion) {
+      assert(!X86AlignBranchWithin32BBoundaries &&
+             "X86AlignForMacroFusion is incompatible with "
+             "X86AlignBranchWithin32BBoundaries");
+      AlignBoundary = assumeAligned(64);
+      AlignBranchType.addKind(X86::AlignBranchFused);
+      AlignBranchType.addKind(X86::AlignBranchJcc);
----------------
In fact, X86AlignForMacroFusion is compatible with X86AlignBranchWithin32BBoundaries, you can set AlignBoundary to 64 and add  AlignBranchFused to AlignBranchType if both of the options exist.


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

https://reviews.llvm.org/D101817