Adjust minRegs for SELECT stress modes

My recent refactoring to put all the `minRegCandidateCount` computation in one place failed to take into account that the `LSRA_SELECT` stress modes can also overly constrain the number of registers. Therefore, it needs to be applied only after the `minRegCandidateCount` has been computed. This may in turn cause new conflicts between the def and use of a tree temp, so factor out the code that checks for that.

Fix #15932

2 files changed, 64 insertions, 40 deletions

diff --git a/src/jit/lsra.cpp b/src/jit/lsra.cpp
index 6fd33efbae..b6c8b96486 100644
--- a/src/jit/lsra.cpp
+++ b/src/jit/lsra.cpp
@@ -721,13 +721,7 @@ void LinearScan::applyCalleeSaveHeuristics(RefPosition* rp)
     Interval* theInterval = rp->getInterval();
 
 #ifdef DEBUG
-    if (doReverseCallerCallee())
-    {
-        regMaskTP calleeSaveMask = calleeSaveRegs(theInterval->registerType);
-        rp->registerAssignment =
-            getConstrainedRegMask(rp->registerAssignment, calleeSaveMask, rp->minRegCandidateCount);
-    }
-    else
+    if (!doReverseCallerCallee())
 #endif // DEBUG
     {
         // Set preferences so that this register set will be preferred for earlier refs
@@ -735,6 +729,52 @@ void LinearScan::applyCalleeSaveHeuristics(RefPosition* rp)
     }
 }
 
+//------------------------------------------------------------------------
+// checkConflictingDefUse: Ensure that we have consistent def/use on SDSU temps.
+//
+// Arguments:
+//    useRP - The use RefPosition of a tree temp (SDSU Interval)
+//
+// Notes:
+//    There are a couple of cases where this may over-constrain allocation:
+//    1. In the case of a non-commutative rmw def (in which the rmw source must be delay-free), or
+//    2. In the case where the defining node requires a temp distinct from the target (also a
+//       delay-free case).
+//    In those cases, if we propagate a single-register restriction from the consumer to the producer
+//    the delayed uses will not see a fixed reference in the PhysReg at that position, and may
+//    incorrectly allocate that register.
+//    TODO-CQ: This means that we may often require a copy at the use of this node's result.
+//    This case could be moved to BuildRefPositionsForNode, at the point where the def RefPosition is
+//    created, causing a RefTypeFixedRef to be added at that location. This, however, results in
+//    more PhysReg RefPositions (a throughput impact), and a large number of diffs that require
+//    further analysis to determine benefit.
+//    See Issue #11274.
+//
+void LinearScan::checkConflictingDefUse(RefPosition* useRP)
+{
+    assert(useRP->refType == RefTypeUse);
+    Interval* theInterval = useRP->getInterval();
+    assert(!theInterval->isLocalVar);
+
+    RefPosition* defRP = theInterval->firstRefPosition;
+
+    // All defs must have a valid treeNode, but we check it below to be conservative.
+    assert(defRP->treeNode != nullptr);
+    regMaskTP prevAssignment = defRP->registerAssignment;
+    regMaskTP newAssignment  = (prevAssignment & useRP->registerAssignment);
+    if (newAssignment != RBM_NONE)
+    {
+        if (!isSingleRegister(newAssignment) || !theInterval->hasInterferingUses)
+        {
+            defRP->registerAssignment = newAssignment;
+        }
+    }
+    else
+    {
+        theInterval->hasConflictingDefUse = true;
+    }
+}
+
 void LinearScan::associateRefPosWithInterval(RefPosition* rp)
 {
     Referenceable* theReferent = rp->referent;
@@ -765,38 +805,7 @@ void LinearScan::associateRefPosWithInterval(RefPosition* rp)
             }
             else if (rp->refType == RefTypeUse)
             {
-                // Ensure that we have consistent def/use on SDSU temps.
-                // However, there are a couple of cases where this may over-constrain allocation:
-                // 1. In the case of a non-commutative rmw def (in which the rmw source must be delay-free), or
-                // 2. In the case where the defining node requires a temp distinct from the target (also a
-                //    delay-free case).
-                // In those cases, if we propagate a single-register restriction from the consumer to the producer
-                // the delayed uses will not see a fixed reference in the PhysReg at that position, and may
-                // incorrectly allocate that register.
-                // TODO-CQ: This means that we may often require a copy at the use of this node's result.
-                // This case could be moved to BuildRefPositionsForNode, at the point where the def RefPosition is
-                // created, causing a RefTypeFixedRef to be added at that location. This, however, results in
-                // more PhysReg RefPositions (a throughput impact), and a large number of diffs that require
-                // further analysis to determine benefit.
-                // See Issue #11274.
-                RefPosition* prevRefPosition = theInterval->recentRefPosition;
-                assert(prevRefPosition != nullptr && theInterval->firstRefPosition == prevRefPosition);
-                // All defs must have a valid treeNode, but we check it below to be conservative.
-                assert(prevRefPosition->treeNode != nullptr);
-                regMaskTP prevAssignment = prevRefPosition->registerAssignment;
-                regMaskTP newAssignment  = (prevAssignment & rp->registerAssignment);
-                if (newAssignment != RBM_NONE)
-                {
-                    if (!isSingleRegister(newAssignment) || !theInterval->hasInterferingUses)
-                    {
-                        prevRefPosition->registerAssignment = newAssignment;
-                    }
-                }
-                else
-                {
-                    theInterval->hasConflictingDefUse = true;
-                }
-
+                checkConflictingDefUse(rp);
                 rp->lastUse = true;
             }
         }
@@ -4000,7 +4009,7 @@ void LinearScan::buildRefPositionsForNode(GenTree*                  tree,
 #ifdef DEBUG
     // If we are constraining registers, modify all the RefPositions we've just built to specify the
     // minimum reg count required.
-    if (getStressLimitRegs() != LSRA_LIMIT_NONE)
+    if ((getStressLimitRegs() != LSRA_LIMIT_NONE) || (getSelectionHeuristics() != LSRA_SELECT_DEFAULT))
     {
         // The number of registers required for a tree node is the sum of
         // consume + produce + internalCount.  This is the minimum
@@ -4035,6 +4044,19 @@ void LinearScan::buildRefPositionsForNode(GenTree*                  tree,
                 }
             }
             newRefPosition->minRegCandidateCount = minRegCountForRef;
+            if (newRefPosition->IsActualRef() && doReverseCallerCallee())
+            {
+                Interval* interval       = newRefPosition->getInterval();
+                regMaskTP oldAssignment  = newRefPosition->registerAssignment;
+                regMaskTP calleeSaveMask = calleeSaveRegs(interval->registerType);
+                newRefPosition->registerAssignment =
+                    getConstrainedRegMask(oldAssignment, calleeSaveMask, minRegCountForRef);
+                if ((newRefPosition->registerAssignment != oldAssignment) && (newRefPosition->refType == RefTypeUse) &&
+                    !interval->isLocalVar)
+                {
+                    checkConflictingDefUse(newRefPosition);
+                }
+            }
         }
     }
 #endif // DEBUG
diff --git a/src/jit/lsra.h b/src/jit/lsra.h
index 1fbaaa2d3b..17c6c93a9c 100644
--- a/src/jit/lsra.h
+++ b/src/jit/lsra.h
@@ -1123,6 +1123,8 @@ private:
 
     void applyCalleeSaveHeuristics(RefPosition* rp);
 
+    void checkConflictingDefUse(RefPosition* rp);
+
     void associateRefPosWithInterval(RefPosition* rp);
 
     void associateRefPosWithRegister(RefPosition* rp);