[PATCH] D75438: [X86] Reduce the number of emitted fragments due to branch align

[Kan Shengchen via Phabricator via llvm-commits]

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


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:522
+
+  if (!isMacroFused(PrevInst, Inst))
+    // Macro fusion doesn't happen indeed, clear the pending.
----------------
LuoYuanke wrote:
> PendingBoundaryAlign has been set to nullptr in alignBranchesEnd()?
> 
We need to clear the `PendingBoundaryAlign`
- If macro fusion doesn't happen.
- Or if the `PendingBoundaryAlign` is assigned a fragment

`alignBranchesEnd` is responsible for the second case and  `alignBranchesBegin` is responsible for the first case.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:565
+
+  if (!needAlignInst(Inst) || !PendingBoundaryAlign)
+    return;
----------------
LuoYuanke wrote:
> Only check PendingBoundaryAlign is enough?  In alignBranchesBegin(), if needAlignInst(Inst), a PendingBoundaryAlign is added.
That' not enough. We need to handle the macro fusion case. When `PrevInst` is the first instruction in a fusible pair, the `PendingBoundaryAlign` is not null and we should return directly here.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:570
+  PendingBoundaryAlign->setLastFragment(OS.getCurrentFragment());
+  PendingBoundaryAlign = nullptr;
+
----------------
LuoYuanke wrote:
> After setting PendingBoundaryAlign to nullptr, it seems there is no chance to reset LastFragment of PendingBoundaryAlign for macro fusion case.
The macro fusion case is corretly handled here. We do not need to **reset LastFragment of PendingBoundaryAlign**. Only when the `Inst` needs to be aligned, we can reach here and `LastFragment` is only set once.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp:572
+
+  // We need to ensure that further data isn't added to the current data
+  // fragment. The easiest way is to insert a new empty DataFragment.
----------------
LuoYuanke wrote:
> I don't understand why further data isn't added to the current data. Is it because we need get instruction size? Could you comments it in the code?
Yes, it is because we need get instruction size. I will add it to comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75438