LSRA: remove last uses only at use point

LSRA maintains liveness within a block to determine what's live across a call. It uses the last use bits on lclVar nodes to remove them from the set. However, this should be done at the point of use rather than at the point where the lclVar is encountered in the execution stream.

Fix #17389

diff --git a/src/jit/lsrabuild.cpp b/src/jit/lsrabuild.cpp
index 5023b13..9f825c9 100644 (file)
--- a/src/jit/lsrabuild.cpp
+++ b/src/jit/lsrabuild.cpp
@@ -1346,21 +1346,6 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, BasicBlock* block, Lsra
             assert(varDsc->lvTracked);
             unsigned varIndex = varDsc->lvVarIndex;
 
-            // We have only approximate last-use information at this point.  This is because the
-            // execution order doesn't actually reflect the true order in which the localVars
-            // are referenced - but the order of the RefPositions will, so we recompute it after
-            // RefPositions are built.
-            // Use the old value for setting currentLiveVars - note that we do this with the
-            // not-quite-correct setting of lastUse.  However, this is OK because
-            // 1) this is only for preferencing, which doesn't require strict correctness, and
-            // 2) the cases where these out-of-order uses occur should not overlap a kill.
-            // TODO-Throughput: clean this up once we have the execution order correct.  At that point
-            // we can update currentLiveVars at the same place that we create the RefPosition.
-            if ((tree->gtFlags & GTF_VAR_DEATH) != 0)
-            {
-                VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex);
-            }
-
             if (!tree->IsUnusedValue() && !tree->isContained())
             {
                 assert(produce != 0);
@@ -1435,11 +1420,6 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, BasicBlock* block, Lsra
                     srcInterval->assignRelatedInterval(varDefInterval);
                 }
             }
-
-            if ((tree->gtFlags & GTF_VAR_DEATH) == 0)
-            {
-                VarSetOps::AddElemD(compiler, currentLiveVars, varIndex);
-            }
         }
         else if (store->gtOp1->OperIs(GT_BITCAST))
         {
@@ -1595,9 +1575,34 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, BasicBlock* block, Lsra
 
         assert((candidates & allRegs(srcInterval->registerType)) != 0);
 
-        // For non-localVar uses we record nothing, as nothing needs to be written back to the tree.
-        GenTree* const refPosNode = srcInterval->isLocalVar ? useNode : nullptr;
-        RefPosition*   pos        = newRefPosition(srcInterval, currentLoc, RefTypeUse, refPosNode, candidates, 0);
+        GenTree* refPosNode;
+        if (srcInterval->isLocalVar)
+        {
+            // We have only approximate last-use information at this point.  This is because the
+            // execution order doesn't actually reflect the true order in which the localVars
+            // are referenced - but the order of the RefPositions will, so we recompute it after
+            // RefPositions are built.
+            // Use the old value for setting currentLiveVars - note that we do this with the
+            // not-quite-correct setting of lastUse.  However, this is OK because
+            // 1) this is only for preferencing, which doesn't require strict correctness, and
+            //    for determing which largeVectors require having their upper-half saved & restored.
+            //    (Issue #17481 tracks the issue that this system results in excessive spills and
+            //    should be changed.)
+            // 2) the cases where these out-of-order uses occur should not overlap a kill (they are
+            //    only known to occur within a single expression).
+            if ((useNode->gtFlags & GTF_VAR_DEATH) != 0)
+            {
+                VarSetOps::RemoveElemD(compiler, currentLiveVars, srcInterval->getVarIndex(compiler));
+            }
+            refPosNode = useNode;
+        }
+        else
+        {
+            // For non-localVar uses we record nothing, as nothing needs to be written back to the tree.
+            refPosNode = nullptr;
+        }
+
+        RefPosition* pos = newRefPosition(srcInterval, currentLoc, RefTypeUse, refPosNode, candidates, 0);
         if (delayRegFree)
         {
             pos->delayRegFree = true;
@@ -1735,6 +1740,11 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, BasicBlock* block, Lsra
         else
         {
             assert(registerTypesEquivalent(interval->registerType, registerType));
+            assert(interval->isLocalVar);
+            if ((tree->gtFlags & GTF_VAR_DEATH) == 0)
+            {
+                VarSetOps::AddElemD(compiler, currentLiveVars, interval->getVarIndex(compiler));
+            }
         }
 
         if (prefSrcInterval != nullptr)