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

[Kan Shengchen via Phabricator via llvm-commits]

skan added inline comments.


================
Comment at: llvm/lib/MC/MCAssembler.cpp:1098-1100
+    for (const MCFragment *F = BF.getLastFragment(); F != &BF;
+         F = F->getPrevNode())
+      AlignedSize += computeFragmentSize(Layout, *F);
----------------
Replace tab with space here.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:169-174
+      if (X86AlignForMacroFusion) {
+        // X86AlignBranchWithin32BBoundaries provides a stronger alignment
+        // restriction: that fused pairs don't cross 32B boundary. Turn
+        // X86AlignForMacroFusion off.
+        X86AlignForMacroFusion = false;
+      }
----------------
1. You need test case for this
2. I don't think it's correct to set value to an option in the constructor, maybe you need to define a class member
3. Nest the if clauses here is incorrect.  As I commented before, you should care about the fields set by options rather than the options themselves because the values like `AlignBranchType`, `AlignBoundary` are not set only by X86AlignBranchWithin32BBoundaries.

Maybe a simple solution is to add the following code to the end of the constructor
```
// Clean the alignment request
if (AlignBoundary == Align())
          AlignBranchType.clear();
else if (AlignBranchType)
         AlignBoundary = Align();

If(X86AlignForMacroFusion) {
   if(AlignBoundary == Align())
         AlignForMacroFusionOnly = true;
   if (AlignBoundary > Align(64) || AlignBoundary == Align()) {
          AlignBoundary = assumeAligned(64);
   } 
   AlignBranchType.addKind(X86::AlignBranchFused);         
}
```


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:654-655
 
-  if (needAlign(Inst) || ((AlignBranchType & X86::AlignBranchFused) &&
-                          isFirstMacroFusibleInst(Inst, *MCII))) {
+  bool IsBranchFused = (AlignBranchType & X86::AlignBranchFused) &&
+                       isFirstMacroFusibleInst(Inst, *MCII);
+  if (needAlign(Inst) || IsBranchFused) {
----------------
Name `IsBranchFused` is quite misleading. `Inst` here is not a branch and may not be fused when `IsBranchFused` is true.

You need a better name here, maybe `IsFirstMacroFusibleInstAndMayNeedAlign`?


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:660-661
     OS.insert(PendingBA = new MCBoundaryAlignFragment(AlignBoundary));
+    if (X86AlignForMacroFusion && IsBranchFused)
+      PendingBA->setAvoidEndAlign(true);
   }
----------------
X86AlignForMacroFusion && IsBranchFused  ->  AlignForMacroFusionOnly 
// Add some comments ...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101817