freeCollectInactiveAny should avoid visiting unnecessary intervals

Author: tkrodriguez

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/alloc/lsra/IntervalWalker.java

@@ -72,10 +72,6 @@ protected boolean activateCurrent(@SuppressWarnings({"unused"}) Interval current
         return true;
     }
 
-    void walk() {
-        walkTo(Integer.MAX_VALUE);
-    }
-
     /**
      * Creates a new interval walker.
      *
@@ -178,7 +174,7 @@ private void walkTo(State state, int from, RegisterBinding binding) {
      * @return The next interval or null if there is no {@linkplain #unhandledLists unhandled}
      *         interval at position {@code toOpId}.
      */
-    private Interval nextInterval(int toOpId) {
+    private Interval nextInterval() {
         RegisterBinding binding;
         Interval any = unhandledLists.any;
         Interval fixed = unhandledLists.fixed;
@@ -188,20 +184,13 @@ private Interval nextInterval(int toOpId) {
             binding = !fixed.isEndMarker() && fixed.from() <= any.from() ? RegisterBinding.Fixed : RegisterBinding.Any;
 
             assert binding == RegisterBinding.Fixed && fixed.from() <= any.from() || binding == RegisterBinding.Any && any.from() <= fixed.from() : "wrong interval!!!";
-            assert any.isEndMarker() || fixed.isEndMarker() || any.from() != fixed.from() ||
-                            binding == RegisterBinding.Fixed : "if fixed and any-Interval start at same position, fixed must be processed first";
-
         } else if (!fixed.isEndMarker()) {
             binding = RegisterBinding.Fixed;
         } else {
             return null;
         }
         Interval currentInterval = unhandledLists.get(binding);
 
-        if (toOpId < currentInterval.from()) {
-            return null;
-        }
-
         currentBinding = binding;
         unhandledLists.set(binding, currentInterval.next);
         currentInterval.next = allocator.intervalEndMarker;
@@ -217,9 +206,8 @@ private Interval nextInterval(int toOpId) {
      *                date.
      */
     @SuppressWarnings("try")
-    protected void walkTo(int toOpId) {
-        assert currentPosition <= toOpId : "can not walk backwards";
-        for (Interval currentInterval = nextInterval(toOpId); currentInterval != null; currentInterval = nextInterval(toOpId)) {
+    void walk() {
+        for (Interval currentInterval = nextInterval(); currentInterval != null; currentInterval = nextInterval()) {
             int opId = currentInterval.from();
 
             // set currentPosition prior to call of walkTo
@@ -242,16 +230,7 @@ protected void walkTo(int toOpId) {
             }
         }
         // set currentPosition prior to call of walkTo
-        currentPosition = toOpId;
-
-        if (currentPosition <= allocator.maxOpId()) {
-            // update unhandled stack intervals
-            updateUnhandledStackIntervals(toOpId);
-
-            // call walkTo if still in range
-            walkTo(State.Active, toOpId);
-            walkTo(State.Inactive, toOpId);
-        }
+        currentPosition = Integer.MAX_VALUE;
     }
 
     private void intervalMoved(Interval interval, State from, State to) {
@@ -268,7 +247,7 @@ private void intervalMoved(Interval interval, State from, State to) {
      * #activeLists active}.
      *
      * Note that for {@linkplain RegisterBinding#Fixed fixed} and {@linkplain RegisterBinding#Any
-     * any} intervals this is done in {@link #nextInterval(int)}.
+     * any} intervals this is done in {@link #nextInterval()}.
      */
     private void updateUnhandledStackIntervals(int opId) {
         Interval currentInterval = unhandledLists.get(RegisterBinding.Stack);

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/alloc/lsra/LinearScanRegisterAllocationPhase.java

@@ -24,11 +24,11 @@
  */
 package jdk.graal.compiler.lir.alloc.lsra;
 
-import jdk.graal.compiler.lir.gen.LIRGenerationResult;
-import jdk.graal.compiler.lir.phases.AllocationPhase;
 import org.graalvm.collections.Pair;
-import jdk.graal.compiler.debug.Indent;
 
+import jdk.graal.compiler.debug.Indent;
+import jdk.graal.compiler.lir.gen.LIRGenerationResult;
+import jdk.graal.compiler.lir.phases.AllocationPhase;
 import jdk.vm.ci.code.TargetDescription;
 
 public final class LinearScanRegisterAllocationPhase extends LinearScanAllocationPhase {
@@ -57,12 +57,7 @@ void allocateRegisters() {
             notPrecoloredIntervals = result.getRight();
 
             // allocate cpu registers
-            LinearScanWalker lsw;
-            if (OptimizingLinearScanWalker.Options.LSRAOptimization.getValue(allocator.getOptions())) {
-                lsw = new OptimizingLinearScanWalker(allocator, precoloredIntervals, notPrecoloredIntervals);
-            } else {
-                lsw = new LinearScanWalker(allocator, precoloredIntervals, notPrecoloredIntervals);
-            }
+            LinearScanWalker lsw = new LinearScanWalker(allocator, precoloredIntervals, notPrecoloredIntervals);
             lsw.walk();
             lsw.finishAllocation();
         }

compiler/src/jdk.graal.compiler/src/jdk/graal/compiler/lir/alloc/lsra/LinearScanWalker.java

@@ -66,6 +66,7 @@ class LinearScanWalker extends IntervalWalker {
     private static final CountingTimerKey allocFreeRegister = countingTimer("allocFreeRegister");
     private static final CountingTimerKey activateCurrent = countingTimer("activateCurrent");
     private static final CountingTimerKey splitAndSpillIntersectingIntervals = countingTimer("splitAndSpillIntersectingIntervals");
+    private static final CountingTimerKey allocLockedRegister = countingTimer("allocLockedRegister");
 
     /**
      * The number of split operations on intervals generated by this walker. Splits are inserted
@@ -137,6 +138,11 @@ boolean isActiveRegister(Interval interval) {
         return isActiveRegister(asRegister(interval.location()).number);
     }
 
+    /**
+     * Excludes a given interval from being used by setting its use position to 0 if it is assigned
+     * to an active register. This method is used to mark a register as blocked, indicating that it
+     * cannot be used for allocation.
+     */
     void excludeFromUse(Interval i) {
         Value location = i.location();
         int i1 = asRegister(location).number;
@@ -146,31 +152,29 @@ void excludeFromUse(Interval i) {
     }
 
     void setUsePos(Interval interval, int intersects, boolean onlyProcessUsePos) {
+        int i = asRegister(interval.location()).number;
+        assert isActiveRegister(i) : "should have been check";
         if (intersects != -1) {
             assert intersects != 0 : "must use excludeFromUse to set usePos to 0";
-            int i = asRegister(interval.location()).number;
-            if (isActiveRegister(i)) {
-                if (usePos[i] > intersects) {
-                    usePos[i] = intersects;
-                }
-                if (!onlyProcessUsePos) {
-                    interval.spillNext = spillIntervals[i];
-                    spillIntervals[i] = interval;
-                }
+            if (usePos[i] > intersects) {
+                usePos[i] = intersects;
+            }
+            if (!onlyProcessUsePos) {
+                interval.spillNext = spillIntervals[i];
+                spillIntervals[i] = interval;
             }
         }
     }
 
     private void setUsePosAtIntersection(Interval interval, Interval current) {
-        if (isActiveRegister(interval)) {
-            int i = asRegister(interval.location()).number;
-            int savedUsePos = usePos[i];
-            int intersects = interval.currentIntersectsAtLimit(current, savedUsePos);
-            if (intersects != -1) {
-                assert intersects != 0 : "must use excludeFromUse to set usePos to 0";
-                if (usePos[i] > intersects) {
-                    usePos[i] = intersects;
-                }
+        assert isActiveRegister(interval) : "called should have checked";
+        int i = asRegister(interval.location()).number;
+        int savedUsePos = usePos[i];
+        int intersects = interval.currentIntersectsAtLimit(current, savedUsePos);
+        if (intersects != -1) {
+            assert intersects != 0 : "must use excludeFromUse to set usePos to 0";
+            if (usePos[i] > intersects) {
+                usePos[i] = intersects;
             }
         }
     }
@@ -214,6 +218,12 @@ void freeExcludeActiveAny() {
         }
     }
 
+    /**
+     * Collects information about inactive intervals of type {@link RegisterBinding#Fixed} that
+     * intersect with the given {@code current} interval. The method iterates through the inactive
+     * intervals, checks if they are assigned to an active register, and updates the use position
+     * information accordingly.
+     */
     @SuppressWarnings("try")
     void freeCollectInactiveFixed(Interval current) {
         try (DebugCloseable t = allocator.start(freeCollectInactiveFixed)) {
@@ -222,7 +232,7 @@ void freeCollectInactiveFixed(Interval current) {
                 if (isActiveRegister(interval)) {
                     int currentFrom = interval.currentFrom();
                     if (current.to() <= currentFrom) {
-                        assert !(interval.currentIntersectsAt(current) != -1) : "must not intersect";
+                        assert interval.currentIntersectsAt(current) == -1 : "must not intersect";
                         int i = asRegister(interval.location()).number;
                         if (usePos[i] > currentFrom) {
                             usePos[i] = currentFrom;
@@ -236,19 +246,44 @@ void freeCollectInactiveFixed(Interval current) {
         }
     }
 
+    /**
+     * Collects information about inactive intervals of type {@link RegisterBinding#Any} that
+     * intersect with the given {@code current} interval. The method iterates through the inactive
+     * intervals, checks if they are assigned to an active register, and updates the use position
+     * information accordingly. The iteration stops when an interval is encountered whose
+     * {@linkplain Interval#currentFrom() current from} position exceeds the specified
+     * {@code limit}. This avoids potentially useless work that can become expensive for larger
+     * compiles.
+     */
     @SuppressWarnings("try")
-    void freeCollectInactiveAny(Interval current) {
+    void freeCollectInactiveAny(Interval current, int limit) {
         try (DebugCloseable t = allocator.start(freeCollectInactiveAny)) {
             Interval interval = inactiveLists.get(RegisterBinding.Any);
             while (!interval.isEndMarker()) {
                 if (isActiveRegister(interval)) {
                     setUsePosAtIntersection(interval, current);
                 }
+                if (interval.currentFrom() > limit) {
+                    /*
+                     * All following intervals must be greater than limit as activeLists and
+                     * inactiveLists are sorted by currentFrom. See
+                     * #addToListSortedByCurrentFromPositions.
+                     */
+                    return;
+                }
                 interval = interval.next;
             }
         }
     }
 
+    /**
+     * Excludes all {@linkplain RegisterBinding#Fixed fixed} intervals currently
+     * {@linkplain State#Active active} from being used. This is done by iterating over the
+     * {@linkplain #activeLists active} intervals with a {@linkplain RegisterBinding#Fixed fixed}
+     * binding and calling {@link #excludeFromUse(Interval)} on each one.
+     *
+     * @see #excludeFromUse(Interval)
+     */
     @SuppressWarnings("try")
     void spillExcludeActiveFixed() {
         try (DebugCloseable t = allocator.start(spillExcludeActiveFixed)) {
@@ -268,7 +303,7 @@ void spillBlockInactiveFixed(Interval current) {
                 if (current.to() > interval.currentFrom()) {
                     setBlockPos(interval, current);
                 } else {
-                    assert !(interval.currentIntersectsAt(current) != -1) : "invalid optimization: intervals intersect";
+                    assert interval.currentIntersectsAt(current) == -1 : "invalid optimization: intervals intersect";
                 }
 
                 interval = interval.next;
@@ -801,18 +836,41 @@ boolean allocFreeRegister(Interval interval) {
         DebugContext debug = allocator.getDebug();
         try (Indent indent = debug.logAndIndent("trying to find free register for %s", interval); DebugCloseable t = allocator.start(allocFreeRegister)) {
 
+            /*
+             * When allocating a partial register, we want it to be free at least until this
+             * position. If the interval has already been split, we add 2 as a heuristic to only
+             * consider registers free for at least two instructions. This is meant to avoid
+             * shuffling data between registers after every single instruction. If register pressure
+             * is so high that we would only get a partial register for the duration of one
+             * instruction and then have to split again, prefer not choosing such a register at all.
+             * We will spill instead.
+             */
+            int partialRegRequestedUntil = interval.from() + 1 + (interval.isSplitChild() ? 2 : 0);
+            int intervalTo = interval.to();
+
             initUseLists(true);
+            /* Mark the active registers as busy */
             freeExcludeActiveFixed();
             freeExcludeActiveAny();
+            /*
+             * Compute the earliest use position of inactive intervals that intersect with this
+             * interval. These are intervals that might become active in the future.
+             */
             freeCollectInactiveFixed(interval);
-            freeCollectInactiveAny(interval);
-            // freeCollectUnhandled(fixedKind, cur);
+            /*
+             * The inactive any list can in some cases be very long which can make this step
+             * expensive, so pass in the minimum usePos required to satisfy this interval.
+             */
+            freeCollectInactiveAny(interval, Math.max(partialRegRequestedUntil, intervalTo));
+
             assert unhandledLists.get(RegisterBinding.Fixed).isEndMarker() : "must not have unhandled fixed intervals because all fixed intervals have a use at position 0";
 
-            // usePos contains the start of the next interval that has this register assigned
-            // (either as a fixed register or a normal allocated register in the past)
-            // only intervals overlapping with cur are processed, non-overlapping invervals can be
-            // ignored safely
+            /*
+             * usePos contains the start of the next interval that has this register assigned
+             * (either as a fixed register or a normal allocated register in the past) only
+             * intervals overlapping with cur are processed, non-overlapping invervals can be
+             * ignored safely
+             */
             if (debug.isLogEnabled()) {
                 // Enable this logging to see all register states
                 try (Indent indent2 = debug.logAndIndent("state of registers:")) {
@@ -833,18 +891,6 @@ boolean allocFreeRegister(Interval interval) {
             }
             assert interval.location() == null : "register already assigned to interval";
 
-            /*
-             * When allocating a partial register, we want it to be free at least until this
-             * position. If the interval has already been split, we add 2 as a heuristic to only
-             * consider registers free for at least two instructions. This is meant to avoid
-             * shuffling data between registers after every single instruction. If register pressure
-             * is so high that we would only get a partial register for the duration of one
-             * instruction and then have to split again, prefer not choosing such a register at all.
-             * We will spill instead.
-             */
-            int partialRegRequestedUntil = interval.from() + 1 + (interval.isSplitChild() ? 2 : 0);
-            int intervalTo = interval.to();
-
             boolean needSplit = false;
             int splitPos = -1;
 
@@ -907,7 +953,8 @@ void splitAndSpillIntersectingIntervals(Register reg) {
     @SuppressWarnings("try")
     void allocLockedRegister(Interval interval) {
         DebugContext debug = allocator.getDebug();
-        try (Indent indent = debug.logAndIndent("alloc locked register: need to split and spill to get register for %s", interval)) {
+        try (Indent indent = debug.logAndIndent("alloc locked register: need to split and spill to get register for %s", interval);
+                        DebugCloseable t = allocator.start(allocLockedRegister)) {
 
             // the register must be free at least until this position
             int firstUsage = interval.firstUsage(RegisterPriority.MustHaveRegister);