Register allocation cleanup (#44977)

* Register allocation cleanup

- Comment updates
- Misc code cleanup

* jit format

diff --git a/docs/design/coreclr/jit/lsra-detail.md b/docs/design/coreclr/jit/lsra-detail.md
index a637b8a..47292da 100644 (file)
--- a/docs/design/coreclr/jit/lsra-detail.md
+++ b/docs/design/coreclr/jit/lsra-detail.md
@@ -387,7 +387,7 @@ well as supporting components) in more depth.
 -   `LinearScan::identifyCandidates`
 
     -   This mostly duplicates what is done in
-        `Compiler::lvaMarkLocalVars(). There are differences in what
+        `Compiler::lvaMarkLocalVars()`. There are differences in what
         constitutes a register candidate vs. what is tracked, but we
         should probably handle them in `Compiler::lvaMarkLocalVars()`
         when it is called after `Lowering`.
@@ -445,7 +445,7 @@ node, which builds `RefPositions` according to the liveness model described abov
 
 -   First, we create `RefPosition`s to define any internal registers that are required.
     As we create new `Interval`s for these, we add the definition `RefPosition` to an array,
-    `InternalDefs`. This allows us to create the corresponding uses of these internal
+    `internalDefs`. This allows us to create the corresponding uses of these internal
     registers later.
 
 -   Then we create `RefPosition`s for each use in the instruction.
@@ -474,7 +474,7 @@ node, which builds `RefPositions` according to the liveness model described abov
          we need to ensure that the other source isn't given the same register as the
          target. For this, we annotate the use `RefPosition` with `delayRegFree`.
 
--   Next we create the uses of the internal registers, using the `InternalDefs` array.
+-   Next we create the uses of the internal registers, using the `internalDefs` array.
     This is cleared before the next instruction is handled.
 
 -   Next, any registers in the kill set for the instruction are killed. This is performed
@@ -652,7 +652,7 @@ LinearScanAllocation(List<RefPosition> refPositions)
         -   When `COMPlus_EnableEHWriteThru == 0`, any variable that's
             live in to an exception region is always referenced on the stack.
 
-        -   See [Enable EHWriteThru by default](#enable-EHWriteThru-by-default).
+        -   See [Enable EHWriteThru by default](#enable-ehwritethru-by-default).
 
 -   Code generation (genGenerateCode)
 

diff --git a/src/coreclr/src/jit/lclvars.cpp b/src/coreclr/src/jit/lclvars.cpp
index 548dc85..1dd05c2 100644 (file)
--- a/src/coreclr/src/jit/lclvars.cpp
+++ b/src/coreclr/src/jit/lclvars.cpp
@@ -4274,7 +4274,7 @@ void Compiler::lvaComputeRefCounts(bool isRecompute, bool setSlotNumbers)
             // If, in minopts/debug, we really want to allow locals to become
             // unreferenced later, we'll have to explicitly clear this bit.
             varDsc->setLvRefCnt(0);
-            varDsc->setLvRefCntWtd(0);
+            varDsc->setLvRefCntWtd(BB_ZERO_WEIGHT);
 
             // Special case for some varargs params ... these must
             // remain unreferenced.

diff --git a/src/coreclr/src/jit/lsra.cpp b/src/coreclr/src/jit/lsra.cpp
index b09b45d..6ee1f39 100644 (file)
--- a/src/coreclr/src/jit/lsra.cpp
+++ b/src/coreclr/src/jit/lsra.cpp
@@ -4179,9 +4179,10 @@ void LinearScan::setIntervalAsSpilled(Interval* interval)
 }
 
 //------------------------------------------------------------------------
-// spill: Spill this Interval between "fromRefPosition" and "toRefPosition"
+// spill: Spill the "interval" starting from "fromRefPosition" (upto "toRefPosition")
 //
 // Arguments:
+//    interval        - The interval that contains the RefPosition to be spilled
 //    fromRefPosition - The RefPosition at which the Interval is to be spilled
 //    toRefPosition   - The RefPosition at which it must be reloaded (debug only arg)
 //
@@ -4372,6 +4373,7 @@ void LinearScan::unassignPhysReg(RegRecord* regRec, RefPosition* spillRefPositio
 {
     Interval* assignedInterval = regRec->assignedInterval;
     assert(assignedInterval != nullptr);
+    assert(spillRefPosition == nullptr || spillRefPosition->getInterval() == assignedInterval);
     regNumber thisRegNum = regRec->regNum;
 
     // Is assignedInterval actually still assigned to this register?

diff --git a/src/coreclr/src/jit/lsra.h b/src/coreclr/src/jit/lsra.h
index c2adcf2..0b1d994 100644 (file)
--- a/src/coreclr/src/jit/lsra.h
+++ b/src/coreclr/src/jit/lsra.h
@@ -371,7 +371,7 @@ class RefInfoListNodePool final
 
 public:
     RefInfoListNodePool(Compiler* compiler, unsigned preallocate = defaultPreallocation);
-    RefInfoListNode* GetNode(RefPosition* r, GenTree* t, unsigned regIdx = 0);
+    RefInfoListNode* GetNode(RefPosition* r, GenTree* t);
     void ReturnNode(RefInfoListNode* listNode);
 };
 
@@ -520,7 +520,7 @@ public:
 
     // interval to which this register is currently allocated.
     // If the interval is inactive (isActive == false) then it is not currently live,
-    // and the register call be unassigned (i.e. setting assignedInterval to nullptr)
+    // and the register can be unassigned (i.e. setting assignedInterval to nullptr)
     // without spilling the register.
     Interval* assignedInterval;
     // Interval to which this register was previously allocated, and which was unassigned
@@ -1494,11 +1494,11 @@ private:
     // i.e. whose consuming node has not yet been handled.
     RefInfoListNodePool listNodePool;
 
-    // The defList is used for the transient RefInfo that is computed by
-    // the Build methods, and used in building RefPositions.
-    // When Def RefPositions are built for a node, their NodeInfo is placed
-    // in the defList. As the consuming node is handled, it moves the NodeInfo
-    // into an ordered useList corresponding to the uses for that node.
+    // When Def RefPositions are built for a node, their RefInfoListNode
+    // (GenTree* to RefPosition* mapping) is placed in the defList.
+    // As the consuming node is handled, it removes the RefInfoListNode from the
+    // defList, use the interval associated with the corresponding Def RefPosition and
+    // use it to build the Use RefPosition.
     RefInfoList defList;
 
     // As we build uses, we may want to preference the next definition (i.e. the register produced
@@ -1925,13 +1925,12 @@ public:
     GenTree*     treeNode;
     unsigned int bbNum;
 
+    LsraLocation nodeLocation;
+
     // Prior to the allocation pass, registerAssignment captures the valid registers
-    // for this RefPosition. An empty set means that any register is valid.  A non-empty
-    // set means that it must be one of the given registers (may be the full set if the
-    // only constraint is that it must reside in SOME register)
+    // for this RefPosition.
     // After the allocation pass, this contains the actual assignment
-    LsraLocation nodeLocation;
-    regMaskTP    registerAssignment;
+    regMaskTP registerAssignment;
 
     RefType refType;
 

diff --git a/src/coreclr/src/jit/lsrabuild.cpp b/src/coreclr/src/jit/lsrabuild.cpp
index 7b581f2..49e486b 100644 (file)
--- a/src/coreclr/src/jit/lsrabuild.cpp
+++ b/src/coreclr/src/jit/lsrabuild.cpp
@@ -103,15 +103,13 @@ RefInfoListNodePool::RefInfoListNodePool(Compiler* compiler, unsigned preallocat
 //                                    pool.
 //
 // Arguments:
-//    l -    - The `LsraLocation` for the `RefInfo` value.
-//    i      - The interval for the `RefInfo` value.
-//    t      - The IR node for the `RefInfo` value
-//    regIdx - The register index for the `RefInfo` value.
+//    r - The `RefPosition` for the `RefInfo` value.
+//    t - The IR node for the `RefInfo` value
 //
 // Returns:
 //    A pooled or newly-allocated `RefInfoListNode`, depending on the
 //    contents of the pool.
-RefInfoListNode* RefInfoListNodePool::GetNode(RefPosition* r, GenTree* t, unsigned regIdx)
+RefInfoListNode* RefInfoListNodePool::GetNode(RefPosition* r, GenTree* t)
 {
     RefInfoListNode* head = m_freeList;
     if (head == nullptr)
@@ -1695,11 +1693,11 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, BasicBlock* block, Lsra
 
 #ifdef DEBUG
     int newDefListCount = defList.Count();
-    int produce         = newDefListCount - oldDefListCount;
+    // Currently produce is unused, but need to strengthen an assert to check if produce is
+    // as expected. See https://github.com/dotnet/runtime/issues/8678
+    int produce = newDefListCount - oldDefListCount;
     assert((consume == 0) || (ComputeAvailableSrcCount(tree) == consume));
-#endif // DEBUG
 
-#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) || (getSelectionHeuristics() != LSRA_SELECT_DEFAULT))
@@ -2095,7 +2093,6 @@ void LinearScan::buildIntervals()
     {
         setBlockSequence();
     }
-    curBBNum = blockSequence[bbSeqCount - 1]->bbNum;
 
     // Next, create ParamDef RefPositions for all the tracked parameters, in order of their varIndex.
     // Assign these RefPositions to the (nonexistent) BB0.
@@ -2266,7 +2263,7 @@ void LinearScan::buildIntervals()
                     {
                         JITDUMP("  Marking RP #%d of V%02u as spillAfter\n", interval->recentRefPosition->rpNum,
                                 interval->varNum);
-                        interval->recentRefPosition->spillAfter;
+                        interval->recentRefPosition->spillAfter = true;
                     }
                 }
             }