MC: Simplify fragment reuse determination

[MaskRay]

First, avoid checking MCSubtargetInfo by reducing unnecessary overhead
introduced in https://reviews.llvm.org/D44928 . That change passed STI
to both FT_Data and FT_Relaxable fragments, but STI is only necessary
for FT_Relaxable.

The use of STI in FT_Data was added for:

• Bundle alignment mode, which has been removed (MC: Remove bundle alignment mode #148781).
• ARM, which inappropriately uses STI in ARMAsmBackend::applyFixup due
  to tech debt, unlike other targets. All tests passed even without the
  copySTI change.

To ensure safety, copySTI now starts a new fragment to prevent mixed
STI values.

Second, avoid checking LinkerRelaxable by eagerly starting a new
fragment when a FT_Data/FT_Align fragment is marked linker-relaxable.
There is currently an extra empty FT_Data if an alignment immediately
follows a linker-relaxable fragment, which will be improved in the
future when FT_Align information is moved to the variable-tail.

[llvmbot]

@llvm/pr-subscribers-backend-risc-v

@llvm/pr-subscribers-mc

Author: Fangrui Song (MaskRay)

Changes

Reduce unnecessary overhead introduced in
https://reviews.llvm.org/D44928 . That change passed STI to both FT_Data
and FT_Relaxable fragments, but STI is only necessary for FT_Relaxable.

The use of STI in FT_Data was added for:

• Bundle alignment mode, which has been removed (#148781).
• ARM, which inappropriately uses STI in ARMAsmBackend::applyFixup due
  to tech debt, unlike other targets.

To ensure safety, copySTI now starts a new fragment to prevent mixed
STI values.

---

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

6 Files Affected:

• (modified) llvm/include/llvm/MC/MCObjectStreamer.h (+1-12)
• (modified) llvm/include/llvm/MC/MCSection.h (+1)
• (modified) llvm/include/llvm/MC/MCStreamer.h (+3-1)
• (modified) llvm/lib/MC/MCObjectStreamer.cpp (+4-12)
• (modified) llvm/lib/MC/MCParser/MCTargetAsmParser.cpp (+4)
• (modified) llvm/lib/MC/MCStreamer.cpp (+13)

diff --git a/llvm/include/llvm/MC/MCObjectStreamer.h b/llvm/include/llvm/MC/MCObjectStreamer.h
index a55fd4a14675f..319e131999d48 100644
--- a/llvm/include/llvm/MC/MCObjectStreamer.h
+++ b/llvm/include/llvm/MC/MCObjectStreamer.h
@@ -73,20 +73,9 @@ class MCObjectStreamer : public MCStreamer {
   MCSymbol *emitCFILabel() override;
   void emitCFISections(bool EH, bool Debug) override;
 
-  void insert(MCFragment *F) {
-    auto *Sec = CurFrag->getParent();
-    F->setParent(Sec);
-    F->setLayoutOrder(CurFrag->getLayoutOrder() + 1);
-    CurFrag->Next = F;
-    CurFrag = F;
-    Sec->curFragList()->Tail = F;
-  }
-
   /// Get a data fragment to write into, creating a new one if the current
   /// fragment is not FT_Data.
-  /// Optionally a \p STI can be passed in so that a new fragment is created
-  /// if the Subtarget differs from the current fragment.
-  MCFragment *getOrCreateDataFragment(const MCSubtargetInfo *STI = nullptr);
+  MCFragment *getOrCreateDataFragment();
 
 protected:
   bool changeSectionImpl(MCSection *Section, uint32_t Subsection);
diff --git a/llvm/include/llvm/MC/MCSection.h b/llvm/include/llvm/MC/MCSection.h
index 296fdd8af0d14..313071ec75033 100644
--- a/llvm/include/llvm/MC/MCSection.h
+++ b/llvm/include/llvm/MC/MCSection.h
@@ -188,6 +188,7 @@ class LLVM_ABI MCSection {
 // destructors.
 class MCFragment {
   friend class MCAssembler;
+  friend class MCStreamer;
   friend class MCObjectStreamer;
   friend class MCSection;
 
diff --git a/llvm/include/llvm/MC/MCStreamer.h b/llvm/include/llvm/MC/MCStreamer.h
index b3a9aabd6ece5..4b91dbc794682 100644
--- a/llvm/include/llvm/MC/MCStreamer.h
+++ b/llvm/include/llvm/MC/MCStreamer.h
@@ -429,7 +429,6 @@ class LLVM_ABI MCStreamer {
            CurFrag->getParent() == getCurrentSection().first);
     return CurFrag;
   }
-
   /// Save the current and previous section on the section stack.
   void pushSection() {
     SectionStack.push_back(
@@ -457,6 +456,9 @@ class LLVM_ABI MCStreamer {
 
   MCSymbol *endSection(MCSection *Section);
 
+  void insert(MCFragment *F);
+  void newFragment();
+
   /// Returns the mnemonic for \p MI, if the streamer has access to a
   /// instruction printer and returns an empty string otherwise.
   virtual StringRef getMnemonic(const MCInst &MI) const { return ""; }
diff --git a/llvm/lib/MC/MCObjectStreamer.cpp b/llvm/lib/MC/MCObjectStreamer.cpp
index 67433f2b265e5..8a3e8efee0b36 100644
--- a/llvm/lib/MC/MCObjectStreamer.cpp
+++ b/llvm/lib/MC/MCObjectStreamer.cpp
@@ -106,26 +106,18 @@ void MCObjectStreamer::emitFrames(MCAsmBackend *MAB) {
     MCDwarfFrameEmitter::Emit(*this, MAB, false);
 }
 
-static bool canReuseDataFragment(const MCFragment &F,
-                                 const MCAssembler &Assembler,
-                                 const MCSubtargetInfo *STI) {
+static bool canReuseDataFragment(const MCFragment &F) {
   if (!F.hasInstructions())
     return true;
   // Do not add data after a linker-relaxable instruction. The difference
   // between a new label and a label at or before the linker-relaxable
   // instruction cannot be resolved at assemble-time.
-  if (F.isLinkerRelaxable())
-    return false;
-  // If the subtarget is changed mid fragment we start a new fragment to record
-  // the new STI.
-  return !STI || F.getSubtargetInfo() == STI;
+  return !F.isLinkerRelaxable();
 }
 
-MCFragment *
-MCObjectStreamer::getOrCreateDataFragment(const MCSubtargetInfo *STI) {
+MCFragment *MCObjectStreamer::getOrCreateDataFragment() {
   auto *F = getCurrentFragment();
-  if (F->getKind() != MCFragment::FT_Data ||
-      !canReuseDataFragment(*F, *Assembler, STI)) {
+  if (F->getKind() != MCFragment::FT_Data || !canReuseDataFragment(*F)) {
     F = getContext().allocFragment<MCFragment>();
     insert(F);
   }
diff --git a/llvm/lib/MC/MCParser/MCTargetAsmParser.cpp b/llvm/lib/MC/MCParser/MCTargetAsmParser.cpp
index 665d92eb9a21c..44498f21a9060 100644
--- a/llvm/lib/MC/MCParser/MCTargetAsmParser.cpp
+++ b/llvm/lib/MC/MCParser/MCTargetAsmParser.cpp
@@ -9,6 +9,7 @@
 #include "llvm/MC/MCParser/MCTargetAsmParser.h"
 #include "llvm/MC/MCContext.h"
 #include "llvm/MC/MCRegister.h"
+#include "llvm/MC/MCStreamer.h"
 
 using namespace llvm;
 
@@ -22,6 +23,9 @@ MCTargetAsmParser::~MCTargetAsmParser() = default;
 MCSubtargetInfo &MCTargetAsmParser::copySTI() {
   MCSubtargetInfo &STICopy = getContext().getSubtargetCopy(getSTI());
   STI = &STICopy;
+  // The returned STI will likely be modified. Create a new fragment to avoid
+  // mixed STI values within a fragment.
+  getStreamer().newFragment();
   return STICopy;
 }
 
diff --git a/llvm/lib/MC/MCStreamer.cpp b/llvm/lib/MC/MCStreamer.cpp
index d814ab8880500..c3ecf8fc717f5 100644
--- a/llvm/lib/MC/MCStreamer.cpp
+++ b/llvm/lib/MC/MCStreamer.cpp
@@ -1404,6 +1404,19 @@ MCSymbol *MCStreamer::endSection(MCSection *Section) {
   return Sym;
 }
 
+void MCStreamer::insert(MCFragment *F) {
+  auto *Sec = CurFrag->getParent();
+  F->setParent(Sec);
+  F->setLayoutOrder(CurFrag->getLayoutOrder() + 1);
+  CurFrag->Next = F;
+  CurFrag = F;
+  Sec->curFragList()->Tail = F;
+}
+
+void MCStreamer::newFragment() {
+  insert(getContext().allocFragment<MCFragment>());
+}
+
 static VersionTuple
 targetVersionOrMinimumSupportedOSVersion(const Triple &Target,
                                          VersionTuple TargetVersion) {

[aengelke]

LGTM

[aengelke]

Shouldn't isLinkerRelexable imply hasInstructions so that the first check can be removed?

[MaskRay]

Thanks for pointing this out. Just made setLinkerRelaxable eagerly allocate a new fragment so that we don't need to check isLinkerRelaxable here.

[lenary]

Wasn't the idea of this patch sequence to cut down on zero-sized data fragments? Is there something we can do here to get rid of this one?

[MaskRay]

#149030 (or another change) will remove this empty FT_Data. The fragment mechanism requires a large overhaul and I need to split the changes into pieces. The state after this patch does introduce an extra empty fragment for this corner case of RISC-V.

---

Intel's Jump Conditional Code Erratum https://reviews.llvm.org/D76475 made the improvement a bit complicated. I'll sort it out.