Skip to content

[lld-macho] Refactor BPSectionOrderer with CRTP. NFC #124482

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Conversation

MaskRay
Copy link
Member

@MaskRay MaskRay commented Jan 26, 2025

PR #117514 refactored BPSectionOrderer to be used by the ELF port
but introduced some inefficiency:

  • BPSectionBase/BPSymbol are wrappers around a single pointer.
    The numbers of sections and symbols could be huge, and the extra
    allocations are memory inefficient.
  • Reconstructing the returned DenseMap (since BPSectionBase != InputSectin)
    is wasteful.

This patch refactors BPSectionOrderer with Curiously Recurring Template
Pattern and eliminates the inefficiency. In addition,
symbolToSectionIdxs is removed and rootSymbolToSectionIdxs building
is moved to lld/MachO: while getting sections for symbols is cheap in
Mach-O, it is awkward and inefficient in the ELF port.

While here, add a file-level comment and replace some StringMap<*>
(which copies strings) with DenseMap<CachedHashStringRef, *>.

Created using spr 1.3.5-bogner
@llvmbot
Copy link
Member

llvmbot commented Jan 26, 2025

@llvm/pr-subscribers-lld-coff
@llvm/pr-subscribers-lld-macho

@llvm/pr-subscribers-lld

Author: Fangrui Song (MaskRay)

Changes

PR #117514 refactored BPSectionOrderer to be used by the ELF port
but introduced some inefficiency:

  • BPSectionBase/BPSymbol are wrappers around a single pointer.
    The numbers of sections and symbols could be huge, and the extra
    allocations are memory inefficient.
  • Reconstructing the returned DenseMap (since BPSectionBase != InputSectin)
    is wasteful.

This patch refactors BPSectionOrderer with Curiously Recurring Template
Pattern and eliminates the inefficiency. In addition,
rootSymbolToSectionIdxs building is moved to lld/MachO: while getting
sections for symbols is cheap in Mach-O, it is awkward and inefficient
in the ELF port. The ELF implementation can actually bypass
symbolToSectionIdxs.

While here, add a file-level comment and replace some StringMap&lt;*&gt;
(which copies strings) with DenseMap&lt;CachedHashStringRef, *&gt;.


Patch is 26.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/124482.diff

6 Files Affected:

  • (modified) lld/Common/CMakeLists.txt (-2)
  • (modified) lld/MachO/BPSectionOrderer.cpp (+118-15)
  • (modified) lld/MachO/BPSectionOrderer.h (+1-117)
  • (modified) lld/MachO/CMakeLists.txt (+1)
  • (removed) lld/include/lld/Common/BPSectionOrdererBase.h (-70)
  • (renamed) lld/include/lld/Common/BPSectionOrdererBase.inc (+82-53)
diff --git a/lld/Common/CMakeLists.txt b/lld/Common/CMakeLists.txt
index 43e91b85821dbf..4f503d04f7844f 100644
--- a/lld/Common/CMakeLists.txt
+++ b/lld/Common/CMakeLists.txt
@@ -24,7 +24,6 @@ set_source_files_properties("${version_inc}"
 
 add_lld_library(lldCommon
   Args.cpp
-  BPSectionOrdererBase.cpp
   CommonLinkerContext.cpp
   DriverDispatcher.cpp
   DWARF.cpp
@@ -48,7 +47,6 @@ add_lld_library(lldCommon
   Demangle
   MC
   Option
-  ProfileData
   Support
   Target
   TargetParser
diff --git a/lld/MachO/BPSectionOrderer.cpp b/lld/MachO/BPSectionOrderer.cpp
index c2d03c968534e4..116daa1a2f6cbe 100644
--- a/lld/MachO/BPSectionOrderer.cpp
+++ b/lld/MachO/BPSectionOrderer.cpp
@@ -8,39 +8,142 @@
 
 #include "BPSectionOrderer.h"
 #include "InputSection.h"
+#include "Relocations.h"
+#include "Symbols.h"
+#include "lld/Common/BPSectionOrdererBase.inc"
 #include "llvm/ADT/DenseMap.h"
+#include "llvm/ADT/StableHashing.h"
+#include "llvm/Support/Endian.h"
+#include "llvm/Support/xxhash.h"
 
 #define DEBUG_TYPE "bp-section-orderer"
 
 using namespace llvm;
 using namespace lld::macho;
 
+namespace {
+struct BPOrdererMachO;
+}
+template <> struct lld::BPOrdererTraits<struct BPOrdererMachO> {
+  using Section = macho::InputSection;
+  using Symbol = macho::Symbol;
+};
+namespace {
+struct BPOrdererMachO : lld::BPOrderer<BPOrdererMachO> {
+  static uint64_t getSize(const Section &sec) { return sec.getSize(); }
+  static bool isCodeSection(const Section &sec) {
+    return macho::isCodeSection(&sec);
+  }
+  static SmallVector<Symbol *, 0> getSymbols(const Section &sec) {
+    SmallVector<Symbol *, 0> symbols;
+    for (auto *sym : sec.symbols)
+      if (auto *d = llvm::dyn_cast_or_null<Defined>(sym))
+        symbols.emplace_back(d);
+    return symbols;
+  }
+
+  // Linkage names can be prefixed with "_" or "l_" on Mach-O. See
+  // Mangler::getNameWithPrefix() for details.
+  std::optional<StringRef> static getResolvedLinkageName(llvm::StringRef name) {
+    if (name.consume_front("_") || name.consume_front("l_"))
+      return name;
+    return {};
+  }
+
+  static void
+  getSectionHashes(const Section &sec, llvm::SmallVectorImpl<uint64_t> &hashes,
+                   const llvm::DenseMap<const void *, uint64_t> &sectionToIdx) {
+    constexpr unsigned windowSize = 4;
+
+    // Calculate content hashes: k-mers and the last k-1 bytes.
+    ArrayRef<uint8_t> data = sec.data;
+    if (data.size() >= windowSize)
+      for (size_t i = 0; i <= data.size() - windowSize; ++i)
+        hashes.push_back(llvm::support::endian::read32le(data.data() + i));
+    for (uint8_t byte : data.take_back(windowSize - 1))
+      hashes.push_back(byte);
+
+    // Calculate relocation hashes
+    for (const auto &r : sec.relocs) {
+      if (r.length == 0 || r.referent.isNull() || r.offset >= data.size())
+        continue;
+
+      uint64_t relocHash = getRelocHash(r, sectionToIdx);
+      uint32_t start = (r.offset < windowSize) ? 0 : r.offset - windowSize + 1;
+      for (uint32_t i = start; i < r.offset + r.length; i++) {
+        auto window = data.drop_front(i).take_front(windowSize);
+        hashes.push_back(xxh3_64bits(window) ^ relocHash);
+      }
+    }
+
+    llvm::sort(hashes);
+    hashes.erase(std::unique(hashes.begin(), hashes.end()), hashes.end());
+  }
+
+  static llvm::StringRef getSymName(const Symbol &sym) { return sym.getName(); }
+  static uint64_t getSymValue(const Symbol &sym) {
+    if (auto *d = dyn_cast<Defined>(&sym))
+      return d->value;
+    return 0;
+  }
+  static uint64_t getSymSize(const Symbol &sym) {
+    if (auto *d = dyn_cast<Defined>(&sym))
+      return d->size;
+    return 0;
+  }
+
+private:
+  static uint64_t
+  getRelocHash(const Reloc &reloc,
+               const llvm::DenseMap<const void *, uint64_t> &sectionToIdx) {
+    auto *isec = reloc.getReferentInputSection();
+    std::optional<uint64_t> sectionIdx;
+    if (auto it = sectionToIdx.find(isec); it != sectionToIdx.end())
+      sectionIdx = it->second;
+    uint64_t kind = -1, value = 0;
+    if (isec)
+      kind = uint64_t(isec->kind());
+
+    if (auto *sym = reloc.referent.dyn_cast<Symbol *>()) {
+      kind = (kind << 8) | uint8_t(sym->kind());
+      if (auto *d = llvm::dyn_cast<Defined>(sym))
+        value = d->value;
+    }
+    return llvm::stable_hash_combine(kind, sectionIdx.value_or(0), value,
+                                     reloc.addend);
+  }
+};
+} // namespace
+
 DenseMap<const InputSection *, int> lld::macho::runBalancedPartitioning(
     StringRef profilePath, bool forFunctionCompression, bool forDataCompression,
     bool compressionSortStartupFunctions, bool verbose) {
-
-  SmallVector<std::unique_ptr<BPSectionBase>> sections;
+  SmallVector<InputSection *> sections;
+  DenseMap<StringRef, DenseSet<unsigned>> symbolToSectionIdxs;
   for (const auto *file : inputFiles) {
     for (auto *sec : file->sections) {
       for (auto &subsec : sec->subsections) {
         auto *isec = subsec.isec;
-        if (!isec || isec->data.empty() || !isec->data.data())
+        if (!isec || isec->data.empty())
           continue;
-        sections.emplace_back(std::make_unique<BPSectionMacho>(isec));
+        for (auto *sym : BPOrdererMachO::getSymbols(*isec))
+          symbolToSectionIdxs[sym->getName()].insert(sections.size());
+        sections.emplace_back(isec);
       }
     }
   }
 
-  auto reorderedSections = BPSectionBase::reorderSectionsByBalancedPartitioning(
-      profilePath, forFunctionCompression, forDataCompression,
-      compressionSortStartupFunctions, verbose, sections);
-
-  DenseMap<const InputSection *, int> result;
-  for (const auto &[sec, priority] : reorderedSections) {
-    result.try_emplace(
-        static_cast<const InputSection *>(
-            static_cast<const BPSectionMacho *>(sec)->getSection()),
-        priority);
+  DenseMap<CachedHashStringRef, DenseSet<unsigned>> rootSymbolToSectionIdxs;
+  for (auto &[name, sectionIdxs] : symbolToSectionIdxs) {
+    auto rootName = getRootSymbol(name);
+    rootSymbolToSectionIdxs[CachedHashStringRef(rootName)].insert(
+        sectionIdxs.begin(), sectionIdxs.end());
+    if (auto linkageName = BPOrdererMachO::getResolvedLinkageName(rootName))
+      rootSymbolToSectionIdxs[CachedHashStringRef(*linkageName)].insert(
+          sectionIdxs.begin(), sectionIdxs.end());
   }
-  return result;
+  return BPOrdererMachO::reorderSections(
+      profilePath, forFunctionCompression, forDataCompression,
+      compressionSortStartupFunctions, verbose, sections,
+      rootSymbolToSectionIdxs);
 }
diff --git a/lld/MachO/BPSectionOrderer.h b/lld/MachO/BPSectionOrderer.h
index e3e6b12092e204..a27f605cb1180c 100644
--- a/lld/MachO/BPSectionOrderer.h
+++ b/lld/MachO/BPSectionOrderer.h
@@ -14,134 +14,18 @@
 #ifndef LLD_MACHO_BPSECTION_ORDERER_H
 #define LLD_MACHO_BPSECTION_ORDERER_H
 
-#include "InputSection.h"
-#include "Relocations.h"
-#include "Symbols.h"
-#include "lld/Common/BPSectionOrdererBase.h"
 #include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/StableHashing.h"
 #include "llvm/ADT/StringRef.h"
-#include "llvm/Support/Endian.h"
-#include "llvm/Support/xxhash.h"
 
 namespace lld::macho {
-
 class InputSection;
 
-class BPSymbolMacho : public BPSymbol {
-  const Symbol *sym;
-
-public:
-  explicit BPSymbolMacho(const Symbol *s) : sym(s) {}
-
-  llvm::StringRef getName() const override { return sym->getName(); }
-
-  const Defined *asDefined() const {
-    return llvm::dyn_cast_or_null<Defined>(sym);
-  }
-
-  std::optional<uint64_t> getValue() const override {
-    if (auto *d = asDefined())
-      return d->value;
-    return {};
-  }
-
-  std::optional<uint64_t> getSize() const override {
-    if (auto *d = asDefined())
-      return d->size;
-    return {};
-  }
-
-  const Symbol *getSymbol() const { return sym; }
-};
-
-class BPSectionMacho : public BPSectionBase {
-  const InputSection *isec;
-
-public:
-  explicit BPSectionMacho(const InputSection *sec) : isec(sec) {}
-
-  const void *getSection() const override { return isec; }
-
-  uint64_t getSize() const override { return isec->getSize(); }
-
-  bool isCodeSection() const override { return macho::isCodeSection(isec); }
-
-  SmallVector<std::unique_ptr<BPSymbol>> getSymbols() const override {
-    SmallVector<std::unique_ptr<BPSymbol>> symbols;
-    for (auto *sym : isec->symbols)
-      if (auto *d = llvm::dyn_cast_or_null<Defined>(sym))
-        symbols.emplace_back(std::make_unique<BPSymbolMacho>(d));
-    return symbols;
-  }
-
-  // Linkage names can be prefixed with "_" or "l_" on Mach-O. See
-  // Mangler::getNameWithPrefix() for details.
-  std::optional<StringRef>
-  getResolvedLinkageName(llvm::StringRef name) const override {
-    if (name.consume_front("_") || name.consume_front("l_"))
-      return name;
-    return {};
-  }
-
-  void getSectionHashes(llvm::SmallVectorImpl<uint64_t> &hashes,
-                        const llvm::DenseMap<const void *, uint64_t>
-                            &sectionToIdx) const override {
-    constexpr unsigned windowSize = 4;
-
-    // Calculate content hashes: k-mers and the last k-1 bytes.
-    ArrayRef<uint8_t> data = isec->data;
-    if (data.size() >= windowSize)
-      for (size_t i = 0; i <= data.size() - windowSize; ++i)
-        hashes.push_back(llvm::support::endian::read32le(data.data() + i));
-    for (uint8_t byte : data.take_back(windowSize - 1))
-      hashes.push_back(byte);
-
-    // Calculate relocation hashes
-    for (const auto &r : isec->relocs) {
-      if (r.length == 0 || r.referent.isNull() || r.offset >= data.size())
-        continue;
-
-      uint64_t relocHash = getRelocHash(r, sectionToIdx);
-      uint32_t start = (r.offset < windowSize) ? 0 : r.offset - windowSize + 1;
-      for (uint32_t i = start; i < r.offset + r.length; i++) {
-        auto window = data.drop_front(i).take_front(windowSize);
-        hashes.push_back(xxh3_64bits(window) ^ relocHash);
-      }
-    }
-
-    llvm::sort(hashes);
-    hashes.erase(std::unique(hashes.begin(), hashes.end()), hashes.end());
-  }
-
-private:
-  static uint64_t
-  getRelocHash(const Reloc &reloc,
-               const llvm::DenseMap<const void *, uint64_t> &sectionToIdx) {
-    auto *isec = reloc.getReferentInputSection();
-    std::optional<uint64_t> sectionIdx;
-    if (auto it = sectionToIdx.find(isec); it != sectionToIdx.end())
-      sectionIdx = it->second;
-    uint64_t kind = -1, value = 0;
-    if (isec)
-      kind = uint64_t(isec->kind());
-
-    if (auto *sym = reloc.referent.dyn_cast<Symbol *>()) {
-      kind = (kind << 8) | uint8_t(sym->kind());
-      if (auto *d = llvm::dyn_cast<Defined>(sym))
-        value = d->value;
-    }
-    return llvm::stable_hash_combine(kind, sectionIdx.value_or(0), value,
-                                     reloc.addend);
-  }
-};
-
 /// Run Balanced Partitioning to find the optimal function and data order to
 /// improve startup time and compressed size.
 ///
 /// It is important that .subsections_via_symbols is used to ensure functions
 /// and data are in their own sections and thus can be reordered.
-llvm::DenseMap<const lld::macho::InputSection *, int>
+llvm::DenseMap<const InputSection *, int>
 runBalancedPartitioning(llvm::StringRef profilePath,
                         bool forFunctionCompression, bool forDataCompression,
                         bool compressionSortStartupFunctions, bool verbose);
diff --git a/lld/MachO/CMakeLists.txt b/lld/MachO/CMakeLists.txt
index c778fcf7b6fff7..ecf6ce609e59f2 100644
--- a/lld/MachO/CMakeLists.txt
+++ b/lld/MachO/CMakeLists.txt
@@ -50,6 +50,7 @@ add_lld_library(lldMachO
   Object
   Option
   Passes
+  ProfileData
   Support
   TargetParser
   TextAPI
diff --git a/lld/include/lld/Common/BPSectionOrdererBase.h b/lld/include/lld/Common/BPSectionOrdererBase.h
deleted file mode 100644
index bbd05edc5e55ec..00000000000000
--- a/lld/include/lld/Common/BPSectionOrdererBase.h
+++ /dev/null
@@ -1,70 +0,0 @@
-//===- BPSectionOrdererBase.h ---------------------------------------------===//
-//
-// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
-// See https://llvm.org/LICENSE.txt for license information.
-// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
-//
-//===----------------------------------------------------------------------===//
-//
-// This file defines the common interfaces which may be used by
-// BPSectionOrderer.
-//
-//===----------------------------------------------------------------------===//
-
-#ifndef LLD_COMMON_BP_SECTION_ORDERER_BASE_H
-#define LLD_COMMON_BP_SECTION_ORDERER_BASE_H
-
-#include "llvm/ADT/DenseMap.h"
-#include "llvm/ADT/SmallVector.h"
-#include "llvm/ADT/StringRef.h"
-#include "llvm/ADT/Twine.h"
-#include <memory>
-#include <optional>
-
-namespace lld {
-
-class BPSymbol {
-
-public:
-  virtual ~BPSymbol() = default;
-  virtual llvm::StringRef getName() const = 0;
-  virtual std::optional<uint64_t> getValue() const = 0;
-  virtual std::optional<uint64_t> getSize() const = 0;
-};
-
-class BPSectionBase {
-public:
-  virtual ~BPSectionBase() = default;
-  virtual uint64_t getSize() const = 0;
-  virtual bool isCodeSection() const = 0;
-  virtual llvm::SmallVector<std::unique_ptr<BPSymbol>> getSymbols() const = 0;
-  virtual const void *getSection() const = 0;
-  virtual void getSectionHashes(
-      llvm::SmallVectorImpl<uint64_t> &hashes,
-      const llvm::DenseMap<const void *, uint64_t> &sectionToIdx) const = 0;
-  virtual std::optional<llvm::StringRef>
-  getResolvedLinkageName(llvm::StringRef name) const = 0;
-
-  /// Symbols can be appended with "(.__uniq.xxxx)?.llvm.yyyy" where "xxxx" and
-  /// "yyyy" are numbers that could change between builds. We need to use the
-  /// root symbol name before this suffix so these symbols can be matched with
-  /// profiles which may have different suffixes.
-  static llvm::StringRef getRootSymbol(llvm::StringRef Name) {
-    auto [P0, S0] = Name.rsplit(".llvm.");
-    auto [P1, S1] = P0.rsplit(".__uniq.");
-    return P1;
-  }
-
-  /// Reorders sections using balanced partitioning algorithm based on profile
-  /// data.
-  static llvm::DenseMap<const BPSectionBase *, int>
-  reorderSectionsByBalancedPartitioning(
-      llvm::StringRef profilePath, bool forFunctionCompression,
-      bool forDataCompression, bool compressionSortStartupFunctions,
-      bool verbose,
-      llvm::SmallVector<std::unique_ptr<BPSectionBase>> &inputSections);
-};
-
-} // namespace lld
-
-#endif
diff --git a/lld/Common/BPSectionOrdererBase.cpp b/lld/include/lld/Common/BPSectionOrdererBase.inc
similarity index 75%
rename from lld/Common/BPSectionOrdererBase.cpp
rename to lld/include/lld/Common/BPSectionOrdererBase.inc
index 7d26a5fb844835..6c514e335f151d 100644
--- a/lld/Common/BPSectionOrdererBase.cpp
+++ b/lld/include/lld/Common/BPSectionOrdererBase.inc
@@ -1,31 +1,69 @@
-//===- BPSectionOrdererBase.cpp -------------------------------------------===//
+//===- BPSectionOrdererBase.inc ---------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
+//
+// This file defines the common BPSectionOrderer interface using the Curiously
+// Recurring Template Pattern and dispatches to the BalancedPartitioning
+// algorithm implemented in LLVMSupport. The optimized section layout attempts
+// to group similar sections together (resulting in a smaller compressed app
+// size) and utilize a temporal profile file to reduce page faults during
+// program startup.
+//
+// Clients should derive from BPOrderer, providing concrete implementations for
+// section and symbol representations. Include this file in a .cpp file to
+// specialize the template for the derived class.
+//
+//===----------------------------------------------------------------------===//
 
-#include "lld/Common/BPSectionOrdererBase.h"
 #include "lld/Common/ErrorHandler.h"
+#include "llvm/ADT/CachedHashString.h"
+#include "llvm/ADT/DenseMap.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/SetVector.h"
 #include "llvm/ADT/SmallSet.h"
+#include "llvm/ADT/SmallVector.h"
 #include "llvm/ADT/StringMap.h"
+#include "llvm/ADT/StringRef.h"
+#include "llvm/ADT/Twine.h"
 #include "llvm/ProfileData/InstrProfReader.h"
 #include "llvm/Support/BalancedPartitioning.h"
 #include "llvm/Support/TimeProfiler.h"
 #include "llvm/Support/VirtualFileSystem.h"
+#include <memory>
+#include <optional>
 
 #define DEBUG_TYPE "bp-section-orderer"
 
 using namespace llvm;
 using namespace lld;
 
+namespace lld {
+template <class D> struct BPOrdererTraits;
+
+template <class D> struct BPOrderer {
+  using Section = typename BPOrdererTraits<D>::Section;
+  using Symbol = typename BPOrdererTraits<D>::Symbol;
+
+  // Reorder sections using balanced partitioning algorithm based on profile.
+  static auto
+  reorderSections(llvm::StringRef profilePath, bool forFunctionCompression,
+                  bool forDataCompression, bool compressionSortStartupFunctions,
+                  bool verbose, llvm::ArrayRef<Section *> sections,
+                  const DenseMap<CachedHashStringRef, DenseSet<unsigned>>
+                      &rootSymbolToSectionIdxs)
+      -> llvm::DenseMap<const Section *, int>;
+};
+} // namespace lld
+
 using UtilityNodes = SmallVector<BPFunctionNode::UtilityNodeT>;
 
+template <class D>
 static SmallVector<std::pair<unsigned, UtilityNodes>> getUnsForCompression(
-    ArrayRef<const BPSectionBase *> sections,
+    ArrayRef<const typename D::Section *> sections,
     const DenseMap<const void *, uint64_t> &sectionToIdx,
     ArrayRef<unsigned> sectionIdxs,
     DenseMap<unsigned, SmallVector<unsigned>> *duplicateSectionIdxs,
@@ -38,7 +76,7 @@ static SmallVector<std::pair<unsigned, UtilityNodes>> getUnsForCompression(
 
   for (unsigned sectionIdx : sectionIdxs) {
     const auto *isec = sections[sectionIdx];
-    isec->getSectionHashes(hashes, sectionToIdx);
+    D::getSectionHashes(*isec, hashes, sectionToIdx);
     sectionHashes.emplace_back(sectionIdx, std::move(hashes));
     hashes.clear();
   }
@@ -96,36 +134,27 @@ static SmallVector<std::pair<unsigned, UtilityNodes>> getUnsForCompression(
   return sectionUns;
 }
 
-llvm::DenseMap<const BPSectionBase *, int>
-BPSectionBase::reorderSectionsByBalancedPartitioning(
-    llvm::StringRef profilePath, bool forFunctionCompression,
-    bool forDataCompression, bool compressionSortStartupFunctions, bool verbose,
-    SmallVector<std::unique_ptr<BPSectionBase>> &inputSections) {
+/// Symbols can be appended with "(.__uniq.xxxx)?.llvm.yyyy" where "xxxx" and
+/// "yyyy" are numbers that could change between builds. We need to use the
+/// root symbol name before this suffix so these symbols can be matched with
+/// profiles which may have different suffixes.
+inline StringRef getRootSymbol(StringRef name) {
+  auto [P0, S0] = name.rsplit(".llvm.");
+  auto [P1, S1] = P0.rsplit(".__uniq.");
+  return P1;
+}
+
+template <class D>
+auto BPOrderer<D>::reorderSections(
+    StringRef profilePath, bool forFunctionCompression, bool forDataCompression,
+    bool compressionSortStartupFunctions, bool verbose,
+    ArrayRef<Section *> sections,
+    const DenseMap<CachedHashStringRef, DenseSet<unsigned>>
+        &rootSymbolToSectionIdxs) -> DenseMap<const Section *, int> {
   TimeTraceScope timeScope("Setup Balanced Partitioning");
-  SmallVector<const BPSectionBase *> sections;
   DenseMap<const void *, uint64_t> sectionToIdx;
-  StringMap<DenseSet<unsigned>> symbolToSectionIdxs;
-
-  // Process input sections
-  for (const auto &isec : inputSections) {
-    unsigned sectionIdx = sections.size();
-    sectionToIdx.try_emplace(isec->getSection(), sectionIdx);
-    sections.emplace_back(isec.get());
-    for (auto &sym : isec->getSymbols())
-      symbolToSectionIdxs[sym->getName()].insert(sectionIdx);
-  }
-  StringMap<DenseSet<unsigned>> rootSymbolToSectionIdxs;
-  for (auto &entry : symbolToSectionIdxs) {
-    StringRef name = entry.getKey();
-    auto &sectionIdxs = entry.getValue();
-    name = BPSectionBase::getRootSymbol(name);
-    rootSymbolToSectionIdxs[name].insert(sectionIdxs.begin(),
-                                         sectionIdxs.end());
-    if (auto resolvedLinkageName =
-        ...
[truncated]

@MaskRay
Copy link
Member Author

MaskRay commented Jan 26, 2025

@Colibrow

Created using spr 1.3.5-bogner
@Colibrow
Copy link
Contributor

LGTM. It being more clear and efficient.

Created using spr 1.3.5-bogner
@MaskRay MaskRay merged commit e0c7f08 into main Jan 28, 2025
5 of 7 checks passed
@MaskRay MaskRay deleted the users/MaskRay/spr/lld-macho-refactor-bpsectionorderer-with-crtp-nfc branch January 28, 2025 02:25
github-actions bot pushed a commit to arm/arm-toolchain that referenced this pull request Jan 28, 2025
PR #117514 refactored BPSectionOrderer to be used by the ELF port
but introduced some inefficiency:

* BPSectionBase/BPSymbol are wrappers around a single pointer.
  The numbers of sections and symbols could be huge, and the extra
  allocations are memory inefficient.
* Reconstructing the returned DenseMap (since BPSectionBase != InputSectin)
  is wasteful.

This patch refactors BPSectionOrderer with Curiously Recurring Template
Pattern and eliminates the inefficiency. In addition,
`symbolToSectionIdxs` is removed and `rootSymbolToSectionIdxs` building
is moved to lld/MachO: while getting sections for symbols is cheap in
Mach-O, it is awkward and inefficient in the ELF port.

While here, add a file-level comment and replace some `StringMap<*>`
(which copies strings) with `DenseMap<CachedHashStringRef, *>`.

Pull Request: llvm/llvm-project#124482
Colibrow added a commit to Colibrow/llvm-project that referenced this pull request Jan 28, 2025
MaskRay pushed a commit to Colibrow/llvm-project that referenced this pull request Feb 2, 2025
MaskRay pushed a commit to Colibrow/llvm-project that referenced this pull request Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants