Refactor and optimize register allocator and live set computations

[tgiphil]

No description provided.

[Copilot]

Pull request overview

This PR refactors and optimizes several hot paths in the register allocator, primarily around interval tree querying, live-set computation, and live-range position lookups, with additional unit tests added for the delayed interval tree wrapper.

Changes:

• Added allocation-free interval tree search overloads and TrySearchFirstOverlapping APIs; reduced recursion and improved MaxEnd maintenance logic.
• Optimized register allocator live-set computation, kill-site lookup, and move-resolution interval lookups.
• Expanded xUnit coverage for DelayedIntervalTree and adjusted scheduler queue status logging output.

Reviewed changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/IntervalTree.cs	Adds allocation-free search overloads, TrySearchFirstOverlapping, iterative subtree search, and MaxEnd update refactors.
Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/DelayedIntervalTree.cs	Adds short-circuit logic for pending ops, TrySearchFirstOverlapping, allocation-free search overloads, and a ToString() override.
Source/Mosa.Compiler.Framework/RegisterAllocator/LiveRange.cs	Replaces linear scans with binary searches for next/previous def/use lookups and adds helper search routines.
Source/Mosa.Compiler.Framework/RegisterAllocator/BaseRegisterAllocator.cs	Reuses BitArray buffers for global live sets, renames/optimizes kill-site handling, improves virtual-register enumeration, and accelerates move lookup via indexing by start.
Source/Mosa.Compiler.Framework/MethodScheduler.cs	Simplifies queue status CPU logging output.
Source/Mosa.Compiler.Framework.xUnit/DelayedIntervalTreeTests.cs	Adds extensive tests covering pending add/delete behavior, searching, replace behavior, enumeration flushing, and caller-provided result lists.

Comments suppressed due to low confidence (1)

Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/DelayedIntervalTree.cs:111

• In Remove(...), cancellation of a pending add uses endpoint containment checks. If the removal range fully contains the pending add interval, neither endpoint may be inside the pending interval and the pending add won’t be cancelled, even though Remove will later remove the overlapping interval in the underlying IntervalTree. Consider using Overlaps(...) (or explicit equality if Remove is intended to only target exact intervals).

		// If pending add overlaps, cancel it
		if (delayedAdd && (Contains(delayedAddStart, delayedAddEnd, start) || Contains(delayedAddStart, delayedAddEnd, end)))
		{
			delayedAdd = false;
			return;
		}

[Copilot]

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated 4 comments.

[Copilot]

Pull request overview

Copilot reviewed 26 out of 28 changed files in this pull request and generated 1 comment.

Comments suppressed due to low confidence (3)

Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/IntervalTree.cs:520

• IntervalTree.Replace() writes to the node returned by FindInterval without checking for Sentinel. If the interval isn’t present, FindInterval returns Sentinel and this assignment mutates the Sentinel’s Value, which can break future searches/operations. Replace should either no-op or throw when the interval doesn’t exist (and avoid writing to Sentinel).

	public void Replace(int start, int end, T value)
	{
		var node = FindInterval(Root, new Interval(start, end));
		node.Value = value;
	}

Source/Mosa.Compiler.Framework/MethodScheduler.cs:399

• CalculateCpuUsage(): removing the try/catch means any exception from Process.Refresh()/TotalProcessorTime (platform-specific failures, disposed process, permission issues, etc.) will now propagate out of diagnostic reporting and can crash compilation. Please reintroduce exception handling (or otherwise harden this path) so diagnostics can’t take down the compiler.

	private double CalculateCpuUsage(long currentTicks)
	{
		currentProcess.Refresh();
		var currentCpuTime = currentProcess.TotalProcessorTime;
		var cpuTimeDelta = (currentCpuTime - lastCpuTime).TotalMilliseconds;

		var ticksDelta = currentTicks - lastCpuCheckTicks;
		var wallTimeDelta = (ticksDelta / (double)Stopwatch.Frequency) * 1000.0; // Convert to milliseconds

		lastCpuTime = currentCpuTime;
		lastCpuCheckTicks = currentTicks;

		if (wallTimeDelta > 0 && wallTimeDelta < 60000) // Sanity check: < 60 seconds
		{
			// CPU percentage divided by cores to match Task Manager (0-100% scale)
			var cpuPercent = (cpuTimeDelta / wallTimeDelta / processorCount) * 100.0;
			return Math.Clamp(cpuPercent, 0.0, 100.0);
		}

		return 0.0;
	}

Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/DelayedIntervalTree.cs:111

• Remove(): the pending-add cancellation check only tests whether the endpoints of the remove range fall inside the pending add interval. If the remove range fully covers the pending add (e.g., pending [10,20], remove [5,25]), the pending add will not be cancelled and the tree can report incorrect results. Use a true overlap test (like the Overlaps helper) for this condition.

	public void Remove(int start, int end)
	{
		// If pending add overlaps, cancel it
		if (delayedAdd && (Contains(delayedAddStart, delayedAddEnd, start) || Contains(delayedAddStart, delayedAddEnd, end)))
		{
			delayedAdd = false;
			return;
		}

[Copilot]

Pull request overview

Copilot reviewed 31 out of 34 changed files in this pull request and generated 4 comments.

Comments suppressed due to low confidence (1)

Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/DelayedIntervalTree.cs:111

• Remove(start, end) is intended to cancel a pending add when the remove range overlaps it, but the current check only tests whether the remove start/end points fall inside the pending add. This misses cases like pending add [5,10] followed by remove [0,20], leaving delayedAdd active and causing subsequent Contains/Search calls to return results that should have been removed. Consider using the existing Overlaps(...) helper (or an equivalent overlap check) here as well.

	public void Remove(int start, int end)
	{
		// If pending add overlaps, cancel it
		if (delayedAdd && (Contains(delayedAddStart, delayedAddEnd, start) || Contains(delayedAddStart, delayedAddEnd, end)))
		{
			delayedAdd = false;
			return;
		}

[Copilot]

Pull request overview

Copilot reviewed 34 out of 37 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (1)

Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/DelayedIntervalTree.cs:111

• Remove(int start, int end) says it cancels a pending add when it overlaps, but the condition only checks whether the remove range endpoints fall inside the pending add. This misses overlap cases where the pending add is strictly contained within the remove range. Use the same overlap predicate used elsewhere (e.g., Overlaps(delayedAddStart, delayedAddEnd, start, end)) so pending adds are reliably canceled for any overlap.

	public void Remove(int start, int end)
	{
		// If pending add overlaps, cancel it
		if (delayedAdd && (Contains(delayedAddStart, delayedAddEnd, start) || Contains(delayedAddStart, delayedAddEnd, end)))
		{
			delayedAdd = false;
			return;
		}

[Copilot]

ScheduledEntryPoint() name reads like a past-tense state but it performs an action (scheduling). Renaming to ScheduleEntryPoint() would better match intent and the naming of ScheduleRequiredMethods() below.

[Copilot]

CalculateCpuUsage is invoked from diagnostic status reporting; removing the previous try/catch means failures in Process.Refresh() / TotalProcessorTime (platform/permission/process-lifetime edge cases) can now crash compilation just for diagnostics. Please restore exception shielding (and return 0.0 on failure) so queue reporting can never take down the compiler.

[Copilot]

The new allocation-free Search(..., List<T> result) overloads append to the provided list without clearing it. That is easy to misuse when callers reuse a list across queries (results accumulate silently). Consider clearing result at the start of these methods (or explicitly documenting that the list is appended-to, not overwritten, and renaming accordingly).

[Copilot]

Similar to IntervalTree, the allocation-free Search(..., List<T> result) overloads here append into the caller-provided list without clearing it first, which can easily lead to accumulated/stale results when the list is reused. Consider clearing result at method entry (or documenting/renaming to make append semantics explicit).

[Copilot]

ToString() creates an enumerator to check emptiness but never disposes it. If the underlying enumerator ever holds resources, this could leak; even if it doesn’t today, it’s better to avoid the pattern. Consider using using var enumerator = tree.GetEnumerator(); or checking emptiness via a dedicated property/Count if available.

[Copilot]

Pull request overview

Copilot reviewed 35 out of 48 changed files in this pull request and generated 6 comments.

Comments suppressed due to low confidence (6)

Source/Mosa.Workspace.Experiment.Debug/Program.cs:1

• MeasureCompileTime accepts a Stopwatch stopwatch parameter but measures using the static Stopwatch field instead, which makes the parameter ineffective and can produce incorrect timings if a different stopwatch is passed. Use the stopwatch parameter consistently inside the method (or remove the parameter and always use the static field).
  Source/Mosa.Workspace.Experiment.Debug/Program.cs:1
• MeasureCompileTime accepts a Stopwatch stopwatch parameter but measures using the static Stopwatch field instead, which makes the parameter ineffective and can produce incorrect timings if a different stopwatch is passed. Use the stopwatch parameter consistently inside the method (or remove the parameter and always use the static field).
  Source/Mosa.Workspace.Experiment.Debug/Program.cs:1
• These settings are assigned multiple times with conflicting values within SetRequiredSettings(), which makes the effective configuration hard to reason about. Consolidate these assignments so each setting is assigned once (or add a short comment explaining why they must be toggled).
  Source/Mosa.Workspace.Experiment.Debug/Program.cs:1
• These settings are assigned multiple times with conflicting values within SetRequiredSettings(), which makes the effective configuration hard to reason about. Consolidate these assignments so each setting is assigned once (or add a short comment explaining why they must be toggled).
  Source/Mosa.Compiler.Framework/RegisterAllocator/RedBlackTree/DelayedIntervalTree.cs:111
• The “pending add overlaps” check only tests whether start or end is contained within the pending add, which misses overlap cases like removing a range that fully covers the pending add (e.g., pending add [10,20], remove [0,100]). This contradicts the comment and the newer Overlaps(...) logic used elsewhere; use Overlaps(delayedAddStart, delayedAddEnd, start, end) here for correctness.

	public void Remove(int start, int end)
	{
		// If pending add overlaps, cancel it
		if (delayedAdd && (Contains(delayedAddStart, delayedAddEnd, start) || Contains(delayedAddStart, delayedAddEnd, end)))
		{
			delayedAdd = false;
			return;
		}

Source/Mosa.Utility.Configuration/MOSASettings.cs:1

• SetDefaultSettings() no longer assigns a default value to MethodScanner. If MosaSettings instances can be reused across runs, this can leave MethodScanner in a stale state from a prior configuration. Reintroduce an explicit default assignment here (or ensure the property is always reset elsewhere before use).

[Copilot]

PlatformRegistry.Register() previously performed reflection-based discovery and population, but now it only resets the dictionary; this is a breaking semantic change for any consumers relying on auto-discovery. Consider either (a) restoring discovery under a new opt-in method (e.g., RegisterFromLoadedAssemblies()), or (b) renaming this method to reflect its new behavior (e.g., Reset()), and updating call sites accordingly.

[Copilot]

value = null; will not compile (or will violate nullability expectations) if T is not constrained to a nullable reference type. Prefer value = default; (and, if needed, use nullability annotations) or explicitly constrain IntervalTree<T> with where T : class if null is intended as the “not found” sentinel.

[Copilot]

value = null; will not compile (or will violate nullability expectations) if T is not constrained to a nullable reference type. Prefer value = default; (and, if needed, use nullability annotations) or explicitly constrain IntervalTree<T> with where T : class if null is intended as the “not found” sentinel.

[Copilot]

The XML doc says “filling” a caller-provided list, but the implementation appends without clearing. This is easy to misuse when the same list is reused across calls. Either clear the list inside the method, or update the documentation to explicitly state that results are appended and the caller must clear the list if desired.

[Copilot]

The previous implementation guarded CPU usage calculation with a try/catch, but that protection is removed. If Process.Refresh()/TotalProcessorTime throws (platform/permission/process-state issues), it can now propagate and disrupt scheduling/diagnostics. Consider restoring exception handling around this diagnostic-only code path (ideally narrowly scoped to expected exception types).

[Copilot]

This overload calls Setup() while Schedule(MosaMethod method) does not, creating inconsistent behavior and potential redundant work (especially since Compile() also calls Setup()). Align the overloads by either requiring callers to call Setup() explicitly for both, or making both overloads internally ensure setup in the same way (and ensuring Setup() is guaranteed idempotent).