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

[skan]

Summary

Currently, a BoundaryAlign fragment may be inserted after the branch
that needs to be aligned to truncate the current fragment, this fragment is
unused at most of time. To avoid that, we can insert a new empty Data
fragment instead. Non-relaxable instruction is usually emitted into Data
fragment, so the inserted empty Data fragment will be reused at a high
possibility.

[skan]

Found some internal fail tests

[skan]

Inserting a fragment before the current insertion point is problematic now,
avoid doing this

[reames]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

479

I believe you can simply insert a new empty data fragment instead of adding the special state to all data fragments.

Also, you only need to do so if the last fragment was a data fragment.

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

479

If the next intruction is emitted into a relaxable fragment, the empty data fragment will be unused, is it acceptable to you?

[skan]

Rebase and address the review comment

[skan]

Remove unused temp variable

[skan]

Not invalidFragment when we relax BoundaryAlign

[reames]

llvm/lib/MC/MCAssembler.cpp

1022

Do me a favour and leave this line in for this patch. It can be removed in a follow up, but the assumptions about offset calculation caching are subtle and it's clearly conservatively correct to invalidate too much. :)

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

434

Does this need to be a runtime check? Or can it be an assert?

"assert(OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign)" to be clear.

Also, add braces. Even though there's a single statement the block comment deserves clear demarcation.

439

Add to comment: and we'll tie them together in branchBoundaryEnd

454–459

Drop the else since the previous if returned.

472

Might be good to add an assert here about the expected instructions between pending BA and this one.

477

I believe we only need this bit of code if we tied the aligned instruction into a previous BA. (i.e. we can early return above if there's no pending BA.)

479

Yes. This is an idiom we already use in a few places and we can fix them all at once if it becomes an issue.

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

434

yes, we need to check it at runtime. An AlignFragment might be inserted after previous instruction(see the comment below), we need to insert a new BoundaryAlign before the JCC if there is a AlignFragment between the fusible pair.

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

472

How to check the expected instructions here?

[skan]

Address the review comment

[skan]

llvm/test/MC/X86/align-branch-64-negative.s

30–37

Can I remove the second test?

[skan]

Rebase

[skan]

Ping. There are no new comments for two days.

[LuoYuanke]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

429

PendingBoundaryAlign has been set to nullptr in alignBranchesEnd()?

469

Only check PendingBoundaryAlign is enough? In alignBranchesBegin(), if needAlignInst(Inst), a PendingBoundaryAlign is added.

474

After setting PendingBoundaryAlign to nullptr, it seems there is no chance to reset LastFragment of PendingBoundaryAlign for macro fusion case.

476

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?

[skan]

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

429

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.

469

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.

474

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.

476

Yes, it is because we need get instruction size. I will add it to comments.

[skan]

Add comments for why we need to insert a new DataFragment

[LuoYuanke]

LGTM. Better to hold for few days to have Philips and MaskRay review as well.

[skan]

Ping. I applied this patch and passed the chek-all tests and found no new fails when running SPEC. Does this patch look good to you ?@reames @MaskRay

[MaskRay]

llvm/test/MC/X86/align-branch-64-negative.s

30–36

Why is the test deleted?

[skan]

llvm/test/MC/X86/align-branch-64-negative.s

30–36

This test is a negative test and we expect the label is bounded to the end of the call rather than the start of the jmp. As commented in this test file, the label "label_after" is in the right place now because we insert a new empty DataFragment after the call.

[reames]

llvm/test/MC/X86/align-branch-64-negative.s

30–36

Please update the check line instead. I can't tell from the diff or not whether the change made is correct or not.

p.s. Commenting out a test is almost never the right answer.

[reames]

I applied the patch and reviewed the changed code sequence directly. It's more correct than what was there before.

LGTM, but please make sure the test check lines are updated so that the diff is visible before landing.

[skan]

Address review comments

llvm/include/llvm/MC/MCFragment.h

 };
 
 /// 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.
 class MCBoundaryAlignFragment : public MCFragment {
   /// The alignment requirement of the branch to be aligned.
   Align AlignBoundary;
-  /// Flag to indicate whether the branch is fused.  Use in determining the
-  /// region of fragments being aligned.
-  bool Fused : 1;
-  /// Flag to indicate whether NOPs should be emitted.
-  bool EmitNops : 1;
+  /// The last fragment in the set of fragments to be aligned.
+  const MCFragment *LastFragment = nullptr;
   /// The size of the fragment.  The size is lazily set during relaxation, and
   /// is not meaningful before that.
   uint64_t Size = 0;
 
 public:
-  MCBoundaryAlignFragment(Align AlignBoundary = Align(1), bool Fused = false,
-                          bool EmitNops = false, MCSection *Sec = nullptr)
-      : MCFragment(FT_BoundaryAlign, false, Sec), AlignBoundary(AlignBoundary),
-        Fused(Fused), EmitNops(EmitNops) {}
+  MCBoundaryAlignFragment(Align AlignBoundary, MCSection *Sec = nullptr)
+      : MCFragment(FT_BoundaryAlign, false, Sec), AlignBoundary(AlignBoundary) {
+  }
 
   uint64_t getSize() const { return Size; }
   void setSize(uint64_t Value) { Size = Value; }
 
   Align getAlignment() const { return AlignBoundary; }
   void setAlignment(Align Value) { AlignBoundary = Value; }
 
-  bool isFused() const { return Fused; }
-  void setFused(bool Value) { Fused = Value; }
-
-  bool canEmitNops() const { return EmitNops; }
-  void setEmitNops(bool Value) { EmitNops = Value; }
+  const MCFragment *getLastFragment() const { return LastFragment; }
+  void setLastFragment(const MCFragment *F) {
+    assert(!F || getParent() == F->getParent());
+    LastFragment = F;
+  }
 
   static bool classof(const MCFragment *F) {
     return F->getKind() == MCFragment::FT_BoundaryAlign;
   }
 };
 } // end namespace llvm
 
 #endif // LLVM_MC_MCFRAGMENT_H

llvm/lib/MC/MCAssembler.cpp

 static bool needPadding(uint64_t StartAddr, uint64_t Size,
                         Align BoundaryAlignment) {
   return mayCrossBoundary(StartAddr, Size, BoundaryAlignment) ||
          isAgainstBoundary(StartAddr, Size, BoundaryAlignment);
 }
 
 bool MCAssembler::relaxBoundaryAlign(MCAsmLayout &Layout,
                                      MCBoundaryAlignFragment &BF) {
-  // The MCBoundaryAlignFragment that doesn't emit NOP should not be relaxed.
-  if (!BF.canEmitNops())
+  // BoundaryAlignFragment that doesn't need to align any fragment should not be
+  // relaxed.
+  if (!BF.getLastFragment())
     return false;
 
-  uint64_t AlignedOffset = Layout.getFragmentOffset(BF.getNextNode());
+  uint64_t AlignedOffset = Layout.getFragmentOffset(&BF);
   uint64_t AlignedSize = 0;
   const MCFragment *F = BF.getNextNode();
-  // If the branch is unfused, it is emitted into one fragment, otherwise it is
-  // emitted into two fragments at most, the next MCBoundaryAlignFragment(if
-  // exists) also marks the end of the branch.
-  for (auto i = 0, N = BF.isFused() ? 2 : 1;
-       i != N && !isa<MCBoundaryAlignFragment>(F); ++i, F = F->getNextNode()) {
+  for (const MCFragment *F = BF.getLastFragment(); F != &BF;
+       F = F->getPrevNode())
     AlignedSize += computeFragmentSize(Layout, *F);
-  }
-  uint64_t OldSize = BF.getSize();
-  AlignedOffset -= OldSize;
+
   Align BoundaryAlignment = BF.getAlignment();
   uint64_t NewSize = needPadding(AlignedOffset, AlignedSize, BoundaryAlignment)
                          ? offsetToAlignment(AlignedOffset, BoundaryAlignment)
                          : 0U;
-  if (NewSize == OldSize)
+  if (NewSize == BF.getSize())
     return false;
   BF.setSize(NewSize);
   Layout.invalidateFragmentsFrom(&BF);
   return true;
 }
 
 bool MCAssembler::relaxDwarfLineAddr(MCAsmLayout &Layout,
                                      MCDwarfLineAddrFragment &DF) {
   MCContext &Context = Layout.getAssembler().getContext();
   uint64_t OldSize = DF.getContents().size();
   int64_t AddrDelta;

llvm/lib/MC/MCFragment.cpp

LLVM_DUMP_METHOD void MCFragment::dump() const {
   case MCFragment::FT_LEB: {
     const auto *LF = cast<MCLEBFragment>(this);
     OS << "\n       ";
     OS << " Value:" << LF->getValue() << " Signed:" << LF->isSigned();
     break;
   }
   case MCFragment::FT_BoundaryAlign: {
     const auto *BF = cast<MCBoundaryAlignFragment>(this);
-    if (BF->canEmitNops())
-      OS << " (can emit nops to align";
-    if (BF->isFused())
-      OS << " fused branch)";
-    else
-      OS << " unfused branch)";
     OS << "\n       ";
     OS << " BoundarySize:" << BF->getAlignment().value()
+       << " LastFragment:" << BF->getLastFragment()
        << " Size:" << BF->getSize();
     break;
   }
   case MCFragment::FT_SymbolId: {
     const auto *F = cast<MCSymbolIdFragment>(this);
     OS << "\n       ";
     OS << " Sym:" << F->getSymbol();
     break;

llvm/lib/MC/MCObjectStreamer.cpp

MCObjectStreamer::getOrCreateDataFragment(const MCSubtargetInfo *STI) {
   MCDataFragment *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
   if (!F || !CanReuseDataFragment(*F, *Assembler, STI)) {
     F = new MCDataFragment();
     insert(F);
   }
   return F;
 }
 
-MCBoundaryAlignFragment *MCObjectStreamer::getOrCreateBoundaryAlignFragment() {
-  auto *F = dyn_cast_or_null<MCBoundaryAlignFragment>(getCurrentFragment());
-  if (!F || F->canEmitNops()) {
-    F = new MCBoundaryAlignFragment();
-    insert(F);
-  }
-  return F;
-}
-
 void MCObjectStreamer::visitUsedSymbol(const MCSymbol &Sym) {
   Assembler->registerSymbol(Sym);
 }
 
 void MCObjectStreamer::emitCFISections(bool EH, bool Debug) {
   MCStreamer::emitCFISections(EH, Debug);
   EmitEHFrame = EH;
   EmitDebugFrame = Debug;

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

class X86AsmBackend : public MCAsmBackend {
   X86AlignBranchKind AlignBranchType;
   Align AlignBoundary;
 
   bool isMacroFused(const MCInst &Cmp, const MCInst &Jcc) const;
 
   bool needAlign(MCObjectStreamer &OS) const;
   bool needAlignInst(const MCInst &Inst) const;
   MCInst PrevInst;
+  MCBoundaryAlignFragment *PendingBoundaryAlign = nullptr;
 
 public:
   X86AsmBackend(const Target &T, const MCSubtargetInfo &STI)
       : MCAsmBackend(support::little), STI(STI),
         MCII(T.createMCInstrInfo()) {
     if (X86AlignBranchWithin32BBoundaries) {
       // At the moment, this defaults to aligning fused branches, unconditional
       // jumps, and (unfused) conditional jumps with nops.  Both the
return (InstDesc.isConditionalBranch() &&
          (InstDesc.isCall() &&
           (AlignBranchType & X86::AlignBranchCall)) ||
          (InstDesc.isReturn() &&
           (AlignBranchType & X86::AlignBranchRet)) ||
          (InstDesc.isIndirectBranch() &&
           (AlignBranchType & X86::AlignBranchIndirect));
 }
 
-/// Insert MCBoundaryAlignFragment before instructions to align branches.
+/// Insert BoundaryAlignFragment before instructions to align branches.
 void X86AsmBackend::alignBranchesBegin(MCObjectStreamer &OS,
                                        const MCInst &Inst) {
   if (!needAlign(OS))
     return;
 
-  MCFragment *CF = OS.getCurrentFragment();
-  bool NeedAlignFused = AlignBranchType & X86::AlignBranchFused;
-  if (hasInterruptDelaySlot(PrevInst)) {
+  if (hasInterruptDelaySlot(PrevInst))
     // If this instruction follows an interrupt enabling instruction with a one
     // instruction delay, inserting a nop would change behavior.
-  } else if (NeedAlignFused && isMacroFused(PrevInst, Inst) && CF) {
+    return;
+
+  if (!isMacroFused(PrevInst, Inst))
+    // Macro fusion doesn't happen indeed, clear the pending.
+    PendingBoundaryAlign = nullptr;
+
+  if (PendingBoundaryAlign &&
+      OS.getCurrentFragment()->getPrevNode() == PendingBoundaryAlign)
     // Macro fusion actually happens and there is no other fragment inserted
-    // after the previous instruction. NOP can be emitted in PF to align fused
-    // jcc.
-    if (auto *PF =
-            dyn_cast_or_null<MCBoundaryAlignFragment>(CF->getPrevNode())) {
-      const_cast<MCBoundaryAlignFragment *>(PF)->setEmitNops(true);
-      const_cast<MCBoundaryAlignFragment *>(PF)->setFused(true);
-    }
-  } else if (needAlignInst(Inst)) {
+    // after the previous instruction.
+    //
+    // Do nothing here since we already inserted a BoudaryAlign fragment when
+    // we met the first instruction in the fused pair.
+    //
     // Note: When there is at least one fragment, such as MCAlignFragment,
     // inserted after the previous instruction, e.g.
     //
     // \code
     //   cmp %rax %rcx
     //   .align 16
     //   je .Label0
     // \ endcode
     //
     // We will treat the JCC as a unfused branch although it may be fused
     // with the CMP.
-    auto *F = OS.getOrCreateBoundaryAlignFragment();
-    F->setAlignment(AlignBoundary);
-    F->setEmitNops(true);
-    F->setFused(false);
-  } else if (NeedAlignFused && isFirstMacroFusibleInst(Inst, *MCII)) {
-    // We don't know if macro fusion happens until the reaching the next
-    // instruction, so a place holder is put here if necessary.
-    auto *F = OS.getOrCreateBoundaryAlignFragment();
-    F->setAlignment(AlignBoundary);
-  }
+    return;
 
-  PrevInst = Inst;
+  else if (needAlignInst(Inst) || ((AlignBranchType & X86::AlignBranchFused) &&
+                                   isFirstMacroFusibleInst(Inst, *MCII)))
+    // If we meet a unfused branch or the first instuction in a fusiable pair,
+    // insert a BoundaryAlign fragment.
+    OS.insert(PendingBoundaryAlign =
+                  new MCBoundaryAlignFragment(AlignBoundary));
 }
 
-/// Insert a MCBoundaryAlignFragment to mark the end of the branch to be aligned
-/// if necessary.
+/// Set the last fragment to be aligned for the BoundaryAlignFragment.
 void X86AsmBackend::alignBranchesEnd(MCObjectStreamer &OS, const MCInst &Inst) {
   if (!needAlign(OS))
     return;
-  // If the branch is emitted into a MCRelaxableFragment, we can determine the
-  // size of the branch easily in MCAssembler::relaxBoundaryAlign. When the
-  // branch is fused, the fused branch(macro fusion pair) must be emitted into
-  // two fragments. Or when the branch is unfused, the branch must be emitted
-  // into one fragment. The MCRelaxableFragment naturally marks the end of the
-  // fused or unfused branch.
-  // Otherwise, we need to insert a MCBoundaryAlignFragment to mark the end of
-  // the branch. This MCBoundaryAlignFragment may be reused to emit NOP to align
-  // other branch.
-  if (needAlignInst(Inst) && !isa<MCRelaxableFragment>(OS.getCurrentFragment()))
-    OS.insert(new MCBoundaryAlignFragment(AlignBoundary));
+
+  PrevInst = Inst;
+
+  if (!needAlignInst(Inst))
+    return;
+
+  if (PendingBoundaryAlign) {
+    PendingBoundaryAlign->setLastFragment(OS.getCurrentFragment());
+    PendingBoundaryAlign = nullptr;
+  }
+
+  // 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.
+  if (isa_and_nonnull<MCDataFragment>(OS.getCurrentFragment()))
+    OS.insert(new MCDataFragment());
 
   // Update the maximum alignment on the current section if necessary.
   MCSection *Sec = OS.getCurrentSectionOnly();
   if (AlignBoundary.value() > Sec->getAlignment())
     Sec->setAlignment(AlignBoundary);
 }
 
 Optional<MCFixupKind> X86AsmBackend::getFixupKind(StringRef Name) const {

llvm/test/MC/X86/align-branch-64-negative.s

labeled_call_test1:
   int3
   .endr
 label_before:
   callq bar
 
   # In the second test, we have a label which is expected to be bound to the
   # end of the call.  For instance, we want the to associate some data with
   # the return address of the call.
-  # CHECK: labeled_call_test2:
-  # CHECK: 5a: callq
-  # CHECK: 5f: nop
-  # CHECK: 60 label_after
-  # CHECK: 60: jmp
+##  # check: labeled_call_test2:
+##  # check: 5a: callq
+##  # check: 5f: nop
+##  # check: 60 label_after
+##  # check: 60: jmp
+## The label "label_after" will be at address 0x5f now, should we remove the
+## second test?
   .globl  labeled_call_test2
   .p2align  5
 labeled_call_test2:
   .rept 26
   int3
   .endr
   callq bar
 label_after:
   jmp bar