Skip to content

Conversation

@arsenm
Copy link
Contributor

@arsenm arsenm commented Feb 13, 2025

No description provided.

Copy link
Contributor Author

arsenm commented Feb 13, 2025

This stack of pull requests is managed by Graphite. Learn more about stacking.

@arsenm arsenm marked this pull request as ready for review February 13, 2025 13:08
@llvmbot
Copy link
Member

llvmbot commented Feb 13, 2025

@llvm/pr-subscribers-llvm-regalloc

Author: Matt Arsenault (arsenm)

Changes

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

2 Files Affected:

  • (modified) llvm/include/llvm/CodeGen/LiveIntervals.h (+8-4)
  • (modified) llvm/lib/CodeGen/LiveIntervals.cpp (+5-4)
diff --git a/llvm/include/llvm/CodeGen/LiveIntervals.h b/llvm/include/llvm/CodeGen/LiveIntervals.h
index 708917be497ef..65b59a9d8b41c 100644
--- a/llvm/include/llvm/CodeGen/LiveIntervals.h
+++ b/llvm/include/llvm/CodeGen/LiveIntervals.h
@@ -30,6 +30,7 @@
 #include "llvm/CodeGen/SlotIndexes.h"
 #include "llvm/CodeGen/TargetRegisterInfo.h"
 #include "llvm/MC/LaneBitmask.h"
+#include "llvm/Support/Allocator.h"
 #include "llvm/Support/CommandLine.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/ErrorHandling.h"
@@ -64,6 +65,8 @@ class LiveIntervals {
   MachineDominatorTree *DomTree = nullptr;
   std::unique_ptr<LiveIntervalCalc> LICalc;
 
+  BumpPtrAllocator Allocator;
+
   /// Special pool allocator for VNInfo's (LiveInterval val#).
   VNInfo::Allocator VNInfoAllocator;
 
@@ -170,7 +173,7 @@ class LiveIntervals {
   /// Interval removal.
   void removeInterval(Register Reg) {
     auto &Interval = VirtRegIntervals[Reg];
-    delete Interval;
+    Allocator.Deallocate(Interval);
     Interval = nullptr;
   }
 
@@ -416,7 +419,8 @@ class LiveIntervals {
     if (!LR) {
       // Compute missing ranges on demand.
       // Use segment set to speed-up initial computation of the live range.
-      RegUnitRanges[Unit] = LR = new LiveRange(UseSegmentSetForPhysRegs);
+      RegUnitRanges[Unit] = LR = new (Allocator.Allocate<LiveRange>())
+          LiveRange(UseSegmentSetForPhysRegs);
       computeRegUnitRange(*LR, Unit);
     }
     return *LR;
@@ -433,7 +437,7 @@ class LiveIntervals {
   /// Remove computed live range for register unit \p Unit. Subsequent uses
   /// should rely on on-demand recomputation.
   void removeRegUnit(unsigned Unit) {
-    delete RegUnitRanges[Unit];
+    Allocator.Deallocate(RegUnitRanges[Unit]);
     RegUnitRanges[Unit] = nullptr;
   }
 
@@ -481,7 +485,7 @@ class LiveIntervals {
   bool computeDeadValues(LiveInterval &LI,
                          SmallVectorImpl<MachineInstr *> *dead);
 
-  static LiveInterval *createInterval(Register Reg);
+  LiveInterval *createInterval(Register Reg);
 
   void printInstrs(raw_ostream &O) const;
   void dumpInstrs() const;
diff --git a/llvm/lib/CodeGen/LiveIntervals.cpp b/llvm/lib/CodeGen/LiveIntervals.cpp
index 3485a27335f13..f12bf87023979 100644
--- a/llvm/lib/CodeGen/LiveIntervals.cpp
+++ b/llvm/lib/CodeGen/LiveIntervals.cpp
@@ -144,14 +144,14 @@ bool LiveIntervals::invalidate(
 void LiveIntervals::clear() {
   // Free the live intervals themselves.
   for (unsigned i = 0, e = VirtRegIntervals.size(); i != e; ++i)
-    delete VirtRegIntervals[Register::index2VirtReg(i)];
+    Allocator.Deallocate(VirtRegIntervals[Register::index2VirtReg(i)]);
   VirtRegIntervals.clear();
   RegMaskSlots.clear();
   RegMaskBits.clear();
   RegMaskBlocks.clear();
 
   for (LiveRange *LR : RegUnitRanges)
-    delete LR;
+    Allocator.Deallocate(LR);
   RegUnitRanges.clear();
 
   // Release VNInfo memory regions, VNInfo objects don't need to be dtor'd.
@@ -222,7 +222,7 @@ LLVM_DUMP_METHOD void LiveIntervals::dump() const { print(dbgs()); }
 
 LiveInterval *LiveIntervals::createInterval(Register reg) {
   float Weight = reg.isPhysical() ? huge_valf : 0.0F;
-  return new LiveInterval(reg, Weight);
+  return new (Allocator.Allocate<LiveInterval>()) LiveInterval(reg, Weight);
 }
 
 /// Compute the live interval of a virtual register, based on defs and uses.
@@ -374,7 +374,8 @@ void LiveIntervals::computeLiveInRegUnits() {
         LiveRange *LR = RegUnitRanges[Unit];
         if (!LR) {
           // Use segment set to speed-up initial computation of the live range.
-          LR = RegUnitRanges[Unit] = new LiveRange(UseSegmentSetForPhysRegs);
+          LR = RegUnitRanges[Unit] = new (Allocator.Allocate<LiveRange>())
+              LiveRange(UseSegmentSetForPhysRegs);
           NewRanges.push_back(Unit);
         }
         VNInfo *VNI = LR->createDeadDef(Begin, getVNInfoAllocator());

Copy link
Contributor

@jayfoad jayfoad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems reasonable but do you have any performance data? Have you tried it with llvm-compile-time-tracker.com? And does it increase memory usage measurably, since BumpPtrAllocator::Deallocate does nothing?

MachineDominatorTree *DomTree = nullptr;
std::unique_ptr<LiveIntervalCalc> LICalc;

BumpPtrAllocator Allocator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment saying what this is used for?

@arsenm
Copy link
Contributor Author

arsenm commented Feb 13, 2025

Seems reasonable but do you have any performance data?

No, I did this months ago and wanted to find something, but forgot about it and figured the tracker is the easiest way to test. I don't actually know how to submit branches there. @nikic?

Copy link
Contributor

@nikic nikic left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LiveRange is not POD, so isn't this going to leak big time?

You need to use SpecificBumpPtrAllocator if you want to run destructors.

@nikic
Copy link
Contributor

nikic commented Feb 13, 2025

Current max-rss results: https://llvm-compile-time-tracker.com/compare.php?from=8600d89e55b866186a2dad2e2b4c85bd96150375&to=a2ee139249e919ae4a4ace66870685ea333ef30e&stat=max-rss 30% increase for the clang build. Hopefully this is due to not using SpecificBumpPtrAllocator...

@arsenm arsenm force-pushed the users/arsenm/live-intervals-use-bumpptrallocator branch from fd430c4 to c15e46a Compare February 24, 2025 12:35
@nikic
Copy link
Contributor

nikic commented Feb 25, 2025

BumpPtrAllocator Allocator;

// Allocator for VirtRegIntervals
SpecificBumpPtrAllocator<LiveInterval> LIAllocator;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need this for LiveRange as well, as it contains vectors.

I didn't realize we used a singly linked list for storing subranges,
but that seems bad.

I didn't realize we used a singly linked list for storing subranges.
That seems bad and we should probably switch this to an array
Not sure it's worth it for these, there should never be all that
many. We could pre-allocate the maximum size up front.
@arsenm arsenm force-pushed the users/arsenm/live-intervals-use-bumpptrallocator branch from c15e46a to 4fb8fd4 Compare February 26, 2025 16:57
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.

5 participants