[llvm] [MC][ELF] Emit instructions directly into fragment (PR #94950)

[via llvm-commits]

https://github.com/aengelke created https://github.com/llvm/llvm-project/pull/94950

Avoid needless copying of instructions and fixups and directly emit into the fragment small vectors

---

This slightly improves performance, but I'm not sure we want to merge this as-is:

- If we go larger changes in MCFragment and how we store contents, this would need another refactoring anyway.
- I also don't like that MCCompactEncodedInstFragment is not just an optimized DataFragment -- using an MCDataFragment instead caused test failures with wrong label positions. Given that NaCl is approximately dead, I wasn't motivated to investigate...
- I didn't test all backends locally, so I'm relying on the CI that. There might be bugs, because the start offset was previously always zero, but now it isn't.



>From 49f835cb844ef7b17b2dbe2fa300904bfe76f307 Mon Sep 17 00:00:00 2001
From: Alexis Engelke <engelke at in.tum.de>
Date: Sat, 8 Jun 2024 13:56:31 +0200
Subject: [PATCH] [MC][ELF] Emit instructions directly into fragment

Avoid needless copying of instructions and fixups.
---
 llvm/lib/MC/MCELFStreamer.cpp    | 60 ++++++++++++++++++++++----------
 llvm/lib/MC/MCObjectStreamer.cpp |  6 ++--
 2 files changed, 43 insertions(+), 23 deletions(-)

diff --git a/llvm/lib/MC/MCELFStreamer.cpp b/llvm/lib/MC/MCELFStreamer.cpp
index 23e926c3a9d14..61c842ce39f39 100644
--- a/llvm/lib/MC/MCELFStreamer.cpp
+++ b/llvm/lib/MC/MCELFStreamer.cpp
@@ -549,12 +549,6 @@ static void CheckBundleSubtargets(const MCSubtargetInfo *OldSTI,
 void MCELFStreamer::emitInstToData(const MCInst &Inst,
                                    const MCSubtargetInfo &STI) {
   MCAssembler &Assembler = getAssembler();
-  SmallVector<MCFixup, 4> Fixups;
-  SmallString<256> Code;
-  Assembler.getEmitter().encodeInstruction(Inst, Code, Fixups, STI);
-
-  for (auto &Fixup : Fixups)
-    fixSymbolsInTLSFixups(Fixup.getValue());
 
   // There are several possibilities here:
   //
@@ -573,7 +567,16 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
   //   the group, though.
   MCDataFragment *DF;
 
+  // When bundling is enabled, we can't just append to the data fragment, as it
+  // might need to be a MCCompactEncodedInstFragment for zero fixups.
   if (Assembler.isBundlingEnabled()) {
+    SmallVector<MCFixup, 4> Fixups;
+    SmallString<256> Code;
+    Assembler.getEmitter().encodeInstruction(Inst, Code, Fixups, STI);
+
+    for (auto &Fixup : Fixups)
+      fixSymbolsInTLSFixups(Fixup.getValue());
+
     MCSection &Sec = *getCurrentSectionOnly();
     if (Assembler.getRelaxAll() && isBundleLocked()) {
       // If the -mc-relax-all flag is used and we are bundle-locked, we re-use
@@ -596,6 +599,9 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
       // Optimize memory usage by emitting the instruction to a
       // MCCompactEncodedInstFragment when not in a bundle-locked group and
       // there are no fixups registered.
+      //
+      // Apparently, this is not just a performance optimization? Using an
+      // MCDataFragment at this point causes test failures.
       MCCompactEncodedInstFragment *CEIF = new MCCompactEncodedInstFragment();
       insert(CEIF);
       CEIF->getContents().append(Code.begin(), Code.end());
@@ -616,28 +622,44 @@ void MCELFStreamer::emitInstToData(const MCInst &Inst,
     // We're now emitting an instruction in a bundle group, so this flag has
     // to be turned off.
     Sec.setBundleGroupBeforeFirstInst(false);
-  } else {
-    DF = getOrCreateDataFragment(&STI);
+
+    for (auto &Fixup : Fixups) {
+      Fixup.setOffset(Fixup.getOffset() + DF->getContents().size());
+      DF->getFixups().push_back(Fixup);
+    }
+
+    DF->setHasInstructions(STI);
+    if (!Fixups.empty() && Fixups.back().getTargetKind() ==
+                               getAssembler().getBackend().RelaxFixupKind)
+      DF->setLinkerRelaxable();
+
+    DF->getContents().append(Code.begin(), Code.end());
+
+    if (Assembler.getRelaxAll() && !isBundleLocked()) {
+      mergeFragment(getOrCreateDataFragment(&STI), DF);
+      delete DF;
+    }
+    return;
   }
 
-  // Add the fixups and data.
+  DF = getOrCreateDataFragment(&STI);
+
+  // Emit instruction directly into data fragment.
+  size_t FixupStartIndex = DF->getFixups().size();
+  size_t CodeOffset = DF->getContents().size();
+  Assembler.getEmitter().encodeInstruction(Inst, DF->getContents(),
+                                           DF->getFixups(), STI);
+
+  auto Fixups = MutableArrayRef(DF->getFixups()).slice(FixupStartIndex);
   for (auto &Fixup : Fixups) {
-    Fixup.setOffset(Fixup.getOffset() + DF->getContents().size());
-    DF->getFixups().push_back(Fixup);
+    Fixup.setOffset(Fixup.getOffset() + CodeOffset);
+    fixSymbolsInTLSFixups(Fixup.getValue());
   }
 
   DF->setHasInstructions(STI);
   if (!Fixups.empty() && Fixups.back().getTargetKind() ==
                              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) {
diff --git a/llvm/lib/MC/MCObjectStreamer.cpp b/llvm/lib/MC/MCObjectStreamer.cpp
index 0ccade91677a4..6d18bd2995d9d 100644
--- a/llvm/lib/MC/MCObjectStreamer.cpp
+++ b/llvm/lib/MC/MCObjectStreamer.cpp
@@ -477,10 +477,8 @@ void MCObjectStreamer::emitInstToFragment(const MCInst &Inst,
   MCRelaxableFragment *IF = new MCRelaxableFragment(Inst, STI);
   insert(IF);
 
-  SmallString<128> Code;
-  getAssembler().getEmitter().encodeInstruction(Inst, Code, IF->getFixups(),
-                                                STI);
-  IF->getContents().append(Code.begin(), Code.end());
+  getAssembler().getEmitter().encodeInstruction(Inst, IF->getContents(),
+                                                IF->getFixups(), STI);
 }
 
 #ifndef NDEBUG