[MC] Aligned bundling: remove special handling for RelaxAll #95188

MaskRay · 2024-06-12T01:31:35Z

When both aligned bundling and RelaxAll are enabled, bundle padding is
directly written into fragments (https://reviews.llvm.org/D8072).
(The original motivation was memory usage, which has been achieved from
different angles with recent assembler improvement).

The code presents challenges with the work to replace fragment
representation (e.g. #94950 #95077). This patch removes the special
handling. RelaxAll still works but the behavior seems slightly different
as revealed by 2 changed tests. However, most -mc-relax-all tests are
unchanged.

RelaxAll used to be the default for clang -O0. This mode has significant
code size drawbacks and newer Clang doesn't use it (#90013).

flushPendingLabels: The FOffset parameter can be removed: pending labels
will be assigned to the incoming fragment at offset 0.

Created using spr 1.3.5-bogner

llvmbot · 2024-06-12T01:32:08Z

@llvm/pr-subscribers-backend-x86

Author: Fangrui Song (MaskRay)

Changes

When both aligned bundling and RelaxAll are enabled, bundle padding is
directly written into fragments (https://reviews.llvm.org/D8072).
(The original motivation was memory usage, which has been achieved from
different angles with recent assembler improvement).

The code presents challenges with the work to replace fragment
representation (e.g. #94950 #95077). This patch removes the special
handling. RelaxAll still works but the behavior seems slightly different
as revealed by 3 changed tests. However, most -mc-relax-all tests do
not need changes.

RelaxAll used to be the default for clang -O0. This mode has significant
code size drawbacks and newer Clang doesn't use it (#90013).

Full diff: https://github.com/llvm/llvm-project/pull/95188.diff

8 Files Affected:

(modified) llvm/include/llvm/MC/MCELFStreamer.h (-8)
(modified) llvm/include/llvm/MC/MCWasmStreamer.h (-3)
(modified) llvm/lib/MC/MCELFStreamer.cpp (+3-86)
(modified) llvm/lib/MC/MCObjectStreamer.cpp (+2-6)
(modified) llvm/lib/MC/MCWasmStreamer.cpp (-13)
(modified) llvm/test/MC/X86/AlignedBundling/bundle-group-too-large-error.s (+1-1)
(modified) llvm/test/MC/X86/AlignedBundling/misaligned-bundle-group.s (+1-5)
(modified) llvm/test/MC/X86/AlignedBundling/misaligned-bundle.s (+1-6)

diff --git a/llvm/include/llvm/MC/MCELFStreamer.h b/llvm/include/llvm/MC/MCELFStreamer.h
index 1ff029d44d376..5f8ae5ace56fd 100644
--- a/llvm/include/llvm/MC/MCELFStreamer.h
+++ b/llvm/include/llvm/MC/MCELFStreamer.h
@@ -39,7 +39,6 @@ class MCELFStreamer : public MCObjectStreamer {
   /// state management
   void reset() override {
     SeenIdent = false;
-    BundleGroups.clear();
     MCObjectStreamer::reset();
   }
 
@@ -142,14 +141,7 @@ class MCELFStreamer : public MCObjectStreamer {
   void finalizeCGProfileEntry(const MCSymbolRefExpr *&S, uint64_t Offset);
   void finalizeCGProfile();
 
-  /// Merge the content of the fragment \p EF into the fragment \p DF.
-  void mergeFragment(MCDataFragment *, MCDataFragment *);
-
   bool SeenIdent = false;
-
-  /// BundleGroups - The stack of fragments holding the bundle-locked
-  /// instructions.
-  SmallVector<MCDataFragment *, 4> BundleGroups;
 };
 
 MCELFStreamer *createARMELFStreamer(MCContext &Context,
diff --git a/llvm/include/llvm/MC/MCWasmStreamer.h b/llvm/include/llvm/MC/MCWasmStreamer.h
index f58405214a80a..f95628d5e0e5f 100644
--- a/llvm/include/llvm/MC/MCWasmStreamer.h
+++ b/llvm/include/llvm/MC/MCWasmStreamer.h
@@ -73,9 +73,6 @@ class MCWasmStreamer : public MCObjectStreamer {
 
   void fixSymbolsInTLSFixups(const MCExpr *expr);
 
-  /// Merge the content of the fragment \p EF into the fragment \p DF.
-  void mergeFragment(MCDataFragment *, MCDataFragment *);
-
   bool SeenIdent;
 };
 
diff --git a/llvm/lib/MC/MCELFStreamer.cpp b/llvm/lib/MC/MCELFStreamer.cpp
index 23e926c3a9d14..1c8de26d01b30 100644
--- a/llvm/lib/MC/MCELFStreamer.cpp
+++ b/llvm/lib/MC/MCELFStreamer.cpp
@@ -50,44 +50,6 @@ bool MCELFStreamer::isBundleLocked() const {
   return getCurrentSectionOnly()->isBundleLocked();
 }
 
-void MCELFStreamer::mergeFragment(MCDataFragment *DF,
-                                  MCDataFragment *EF) {
-  MCAssembler &Assembler = getAssembler();
-
-  if (Assembler.isBundlingEnabled() && Assembler.getRelaxAll()) {
-    uint64_t FSize = EF->getContents().size();
-
-    if (FSize > Assembler.getBundleAlignSize())
-      report_fatal_error("Fragment can't be larger than a bundle size");
-
-    uint64_t RequiredBundlePadding = computeBundlePadding(
-        Assembler, EF, DF->getContents().size(), FSize);
-
-    if (RequiredBundlePadding > UINT8_MAX)
-      report_fatal_error("Padding cannot exceed 255 bytes");
-
-    if (RequiredBundlePadding > 0) {
-      SmallString<256> Code;
-      raw_svector_ostream VecOS(Code);
-      EF->setBundlePadding(static_cast<uint8_t>(RequiredBundlePadding));
-      Assembler.writeFragmentPadding(VecOS, *EF, FSize);
-
-      DF->getContents().append(Code.begin(), Code.end());
-    }
-  }
-
-  flushPendingLabels(DF, DF->getContents().size());
-
-  for (unsigned i = 0, e = EF->getFixups().size(); i != e; ++i) {
-    EF->getFixups()[i].setOffset(EF->getFixups()[i].getOffset() +
-                                 DF->getContents().size());
-    DF->getFixups().push_back(EF->getFixups()[i]);
-  }
-  if (DF->getSubtargetInfo() == nullptr && EF->getSubtargetInfo())
-    DF->setHasInstructions(*EF->getSubtargetInfo());
-  DF->getContents().append(EF->getContents().begin(), EF->getContents().end());
-}
-
 void MCELFStreamer::initSections(bool NoExecStack, const MCSubtargetInfo &STI) {
   MCContext &Ctx = getContext();
   switchSection(Ctx.getObjectFileInfo()->getTextSection());
@@ -575,24 +537,12 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
 
   if (Assembler.isBundlingEnabled()) {
     MCSection &Sec = *getCurrentSectionOnly();
-    if (Assembler.getRelaxAll() && isBundleLocked()) {
-      // If the -mc-relax-all flag is used and we are bundle-locked, we re-use
-      // the current bundle group.
-      DF = BundleGroups.back();
-      CheckBundleSubtargets(DF->getSubtargetInfo(), &STI);
-    }
-    else if (Assembler.getRelaxAll() && !isBundleLocked())
-      // When not in a bundle-locked group and the -mc-relax-all flag is used,
-      // we create a new temporary fragment which will be later merged into
-      // the current fragment.
-      DF = new MCDataFragment();
-    else if (isBundleLocked() && !Sec.isBundleGroupBeforeFirstInst()) {
+    if (isBundleLocked() && !Sec.isBundleGroupBeforeFirstInst()) {
       // If we are bundle-locked, we re-use the current fragment.
       // The bundle-locking directive ensures this is a new data fragment.
       DF = cast<MCDataFragment>(getCurrentFragment());
       CheckBundleSubtargets(DF->getSubtargetInfo(), &STI);
-    }
-    else if (!isBundleLocked() && Fixups.size() == 0) {
+    } else if (!isBundleLocked() && Fixups.size() == 0) {
       // Optimize memory usage by emitting the instruction to a
       // MCCompactEncodedInstFragment when not in a bundle-locked group and
       // there are no fixups registered.
@@ -631,13 +581,6 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
                              getAssembler().getBackend().RelaxFixupKind)
     DF->setLinkerRelaxable();
   DF->getContents().append(Code.begin(), Code.end());
-
-  if (Assembler.isBundlingEnabled() && Assembler.getRelaxAll()) {
-    if (!isBundleLocked()) {
-      mergeFragment(getOrCreateDataFragment(&STI), DF);
-      delete DF;
-    }
-  }
 }
 
 void MCELFStreamer::emitBundleAlignMode(Align Alignment) {
@@ -659,12 +602,6 @@ void MCELFStreamer::emitBundleLock(bool AlignToEnd) {
   if (!isBundleLocked())
     Sec.setBundleGroupBeforeFirstInst(true);
 
-  if (getAssembler().getRelaxAll() && !isBundleLocked()) {
-    // TODO: drop the lock state and set directly in the fragment
-    MCDataFragment *DF = new MCDataFragment();
-    BundleGroups.push_back(DF);
-  }
-
   Sec.setBundleLockState(AlignToEnd ? MCSection::BundleLockedAlignToEnd
                                     : MCSection::BundleLocked);
 }
@@ -679,27 +616,7 @@ void MCELFStreamer::emitBundleUnlock() {
   else if (Sec.isBundleGroupBeforeFirstInst())
     report_fatal_error("Empty bundle-locked group is forbidden");
 
-  // When the -mc-relax-all flag is used, we emit instructions to fragments
-  // stored on a stack. When the bundle unlock is emitted, we pop a fragment
-  // from the stack a merge it to the one below.
-  if (getAssembler().getRelaxAll()) {
-    assert(!BundleGroups.empty() && "There are no bundle groups");
-    MCDataFragment *DF = BundleGroups.back();
-
-    // FIXME: Use BundleGroups to track the lock state instead.
-    Sec.setBundleLockState(MCSection::NotBundleLocked);
-
-    // FIXME: Use more separate fragments for nested groups.
-    if (!isBundleLocked()) {
-      mergeFragment(getOrCreateDataFragment(DF->getSubtargetInfo()), DF);
-      BundleGroups.pop_back();
-      delete DF;
-    }
-
-    if (Sec.getBundleLockState() != MCSection::BundleLockedAlignToEnd)
-      getOrCreateDataFragment()->setAlignToBundleEnd(false);
-  } else
-    Sec.setBundleLockState(MCSection::NotBundleLocked);
+  Sec.setBundleLockState(MCSection::NotBundleLocked);
 }
 
 void MCELFStreamer::finishImpl() {
diff --git a/llvm/lib/MC/MCObjectStreamer.cpp b/llvm/lib/MC/MCObjectStreamer.cpp
index bf1ce76cdc14b..e3b2801b25329 100644
--- a/llvm/lib/MC/MCObjectStreamer.cpp
+++ b/llvm/lib/MC/MCObjectStreamer.cpp
@@ -215,7 +215,7 @@ static bool canReuseDataFragment(const MCDataFragment &F,
   // When bundling is enabled, we don't want to add data to a fragment that
   // already has instructions (see MCELFStreamer::emitInstToData for details)
   if (Assembler.isBundlingEnabled())
-    return Assembler.getRelaxAll();
+    return false;
   // If the subtarget is changed mid fragment we start a new fragment to record
   // the new STI.
   return !STI || F.getSubtargetInfo() == STI;
@@ -292,8 +292,7 @@ void MCObjectStreamer::emitLabel(MCSymbol *Symbol, SMLoc Loc) {
   // Otherwise queue the label and set its fragment pointer when we emit the
   // next fragment.
   auto *F = dyn_cast_or_null<MCDataFragment>(getCurrentFragment());
-  if (F && !(getAssembler().isBundlingEnabled() &&
-             getAssembler().getRelaxAll())) {
+  if (F) {
     Symbol->setFragment(F);
     Symbol->setOffset(F->getContents().size());
   } else {
@@ -465,9 +464,6 @@ void MCObjectStreamer::emitInstructionImpl(const MCInst &Inst,
 
 void MCObjectStreamer::emitInstToFragment(const MCInst &Inst,
                                           const MCSubtargetInfo &STI) {
-  if (getAssembler().getRelaxAll() && getAssembler().isBundlingEnabled())
-    llvm_unreachable("All instructions should have already been relaxed");
-
   // Always create a new, separate fragment here, because its size can change
   // during relaxation.
   MCRelaxableFragment *IF = new MCRelaxableFragment(Inst, STI);
diff --git a/llvm/lib/MC/MCWasmStreamer.cpp b/llvm/lib/MC/MCWasmStreamer.cpp
index c553ede77555a..8b59a6c3446f9 100644
--- a/llvm/lib/MC/MCWasmStreamer.cpp
+++ b/llvm/lib/MC/MCWasmStreamer.cpp
@@ -39,19 +39,6 @@ using namespace llvm;
 
 MCWasmStreamer::~MCWasmStreamer() = default; // anchor.
 
-void MCWasmStreamer::mergeFragment(MCDataFragment *DF, MCDataFragment *EF) {
-  flushPendingLabels(DF, DF->getContents().size());
-
-  for (unsigned I = 0, E = EF->getFixups().size(); I != E; ++I) {
-    EF->getFixups()[I].setOffset(EF->getFixups()[I].getOffset() +
-                                 DF->getContents().size());
-    DF->getFixups().push_back(EF->getFixups()[I]);
-  }
-  if (DF->getSubtargetInfo() == nullptr && EF->getSubtargetInfo())
-    DF->setHasInstructions(*EF->getSubtargetInfo());
-  DF->getContents().append(EF->getContents().begin(), EF->getContents().end());
-}
-
 void MCWasmStreamer::emitLabel(MCSymbol *S, SMLoc Loc) {
   auto *Symbol = cast<MCSymbolWasm>(S);
   MCObjectStreamer::emitLabel(Symbol, Loc);
diff --git a/llvm/test/MC/X86/AlignedBundling/bundle-group-too-large-error.s b/llvm/test/MC/X86/AlignedBundling/bundle-group-too-large-error.s
index 697b8bf6ab6c0..b3b4d9b6e9324 100644
--- a/llvm/test/MC/X86/AlignedBundling/bundle-group-too-large-error.s
+++ b/llvm/test/MC/X86/AlignedBundling/bundle-group-too-large-error.s
@@ -1,5 +1,5 @@
 # RUN: not --crash llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu %s -o - 2>&1 | FileCheck %s
-# RUN: not --crash llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu -mc-relax-all %s -o - 2>&1 | FileCheck %s
+# RUN: llvm-mc -filetype=obj -triple x86_64-pc-linux-gnu -mc-relax-all %s -o /dev/null
 
 # CHECK: ERROR: Fragment can't be larger than a bundle size
 
diff --git a/llvm/test/MC/X86/AlignedBundling/misaligned-bundle-group.s b/llvm/test/MC/X86/AlignedBundling/misaligned-bundle-group.s
index 6962a2a65960d..92bd9ec016bd5 100644
--- a/llvm/test/MC/X86/AlignedBundling/misaligned-bundle-group.s
+++ b/llvm/test/MC/X86/AlignedBundling/misaligned-bundle-group.s
@@ -3,7 +3,7 @@
 # RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-OPT %s
 # RUN: llvm-mc -filetype=obj -triple i686-pc-linux-gnu -mcpu=pentiumpro -mc-relax-all %s -o - \
 # RUN:   | llvm-objdump -d --no-show-raw-insn - \
-# RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-RELAX %s
+# RUN:   | FileCheck --check-prefixes=CHECK,CHECK-OPT %s
 
         .text
 foo:
@@ -13,11 +13,7 @@ foo:
         .bundle_lock align_to_end
 # CHECK:            1:  nopw %cs:(%eax,%eax)
 # CHECK:            10: nopw %cs:(%eax,%eax)
-# CHECK-RELAX:      1a: nop
-# CHECK-RELAX:      20: nopw %cs:(%eax,%eax)
-# CHECK-RELAX:      2a: nopw %cs:(%eax,%eax)
 # CHECK-OPT:        1b: calll 0x1c
-# CHECK-RELAX:      3b: calll 0x3c
         calll   bar # 5 bytes
         .bundle_unlock
         ret         # 1 byte
diff --git a/llvm/test/MC/X86/AlignedBundling/misaligned-bundle.s b/llvm/test/MC/X86/AlignedBundling/misaligned-bundle.s
index 7a84bffc1821e..0bf5cfd802be9 100644
--- a/llvm/test/MC/X86/AlignedBundling/misaligned-bundle.s
+++ b/llvm/test/MC/X86/AlignedBundling/misaligned-bundle.s
@@ -3,7 +3,7 @@
 # RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-OPT %s
 # RUN: llvm-mc -filetype=obj -triple i686-pc-linux-gnu -mcpu=pentiumpro -mc-relax-all %s -o - \
 # RUN:   | llvm-objdump --no-print-imm-hex -d --no-show-raw-insn - \
-# RUN:   | FileCheck -check-prefix=CHECK -check-prefix=CHECK-RELAX %s
+# RUN:   | FileCheck --check-prefixes=CHECK,CHECK-OPT %s
 
         .text
 foo:
@@ -11,17 +11,12 @@ foo:
         push    %ebp          # 1 byte
         .align  16
 # CHECK:            1:  nopw %cs:(%eax,%eax)
-# CHECK-RELAX:      10: nopw %cs:(%eax,%eax)
-# CHECK-RELAX:      1a: nop
 # CHECK-OPT:        10: movl $1, (%esp)
-# CHECK-RELAX:      20: movl $1, (%esp)
         movl $0x1, (%esp)     # 7 bytes
         movl $0x1, (%esp)     # 7 bytes
 # CHECK-OPT:        1e: nop
         movl $0x2, 0x1(%esp)  # 8 bytes
         movl $0x2, 0x1(%esp)  # 8 bytes
-# CHECK-RELAX:      3e: nop
-# CHECK-RELAX:      40: movl $2, 1(%esp)
         movl $0x2, 0x1(%esp)  # 8 bytes
         movl $0x2, (%esp)     # 7 bytes
 # CHECK-OPT:        3f: nop

…8072 Created using spr 1.3.5-bogner

aengelke

LGTM if RelaxAll with bundling isn't required any more. Love how this simplifies code quite a bit!

Created using spr 1.3.5-bogner

There is only one caller after #95188.

There is only one caller after llvm#95188.

[𝘀𝗽𝗿] initial version

eae4e40

Created using spr 1.3.5-bogner

llvmbot added backend:X86 mc Machine (object) code labels Jun 12, 2024

MaskRay requested review from petrhosek, dschuff and aengelke June 12, 2024 01:32

drop another piece of code that is part of https://reviews.llvm.org/D…

3590a05

…8072 Created using spr 1.3.5-bogner

aengelke approved these changes Jun 12, 2024

View reviewed changes

rebase

f0c7be2

Created using spr 1.3.5-bogner

MaskRay merged commit b1932b8 into main Jun 14, 2024
4 of 6 checks passed

MaskRay deleted the users/MaskRay/spr/mc-aligned-bundling-remove-special-handling-for-relaxall branch June 14, 2024 17:01

MaskRay added a commit that referenced this pull request Jun 22, 2024

[MC] Move computeBundlePadding closer to its only caller. NFC

c9f6a5e

There is only one caller after #95188.

MaskRay mentioned this pull request Jul 3, 2024

[MC][ELF] Emit instructions directly into fragment #94950

Merged

AlexisPerry pushed a commit to llvm-project-tlp/llvm-project that referenced this pull request Jul 9, 2024

[MC] Move computeBundlePadding closer to its only caller. NFC

bdc0747

There is only one caller after llvm#95188.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[MC] Aligned bundling: remove special handling for RelaxAll #95188

[MC] Aligned bundling: remove special handling for RelaxAll #95188

MaskRay commented Jun 12, 2024 •

edited

Loading

llvmbot commented Jun 12, 2024

aengelke left a comment

[MC] Aligned bundling: remove special handling for RelaxAll #95188

[MC] Aligned bundling: remove special handling for RelaxAll #95188

Conversation

MaskRay commented Jun 12, 2024 • edited Loading

llvmbot commented Jun 12, 2024

aengelke left a comment

Choose a reason for hiding this comment

MaskRay commented Jun 12, 2024 •

edited

Loading