From d4521476f0360bc673450c6c0721bae0181c9b6e Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Sun, 9 Apr 2023 10:25:50 -0700 Subject: [PATCH 01/11] Fix issue to handle candidates that don't have assignedInterval in spillcost/prevReg --- src/coreclr/jit/lsra.cpp | 73 +++++++++++++++++++++++----------------- 1 file changed, 43 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 8cd4ee9bf87b01..6703e5db764d54 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -11686,46 +11686,50 @@ void LinearScan::RegisterSelection::try_SPILL_COST() // Can and should the interval in this register be spilled for this one, // if we don't find a better alternative? + weight_t currentSpillWeight = 0; #ifdef TARGET_ARM64 - if (assignedInterval == nullptr) - { - // Ideally we should not be seeing this candidate because it is not assigned to - // any interval. But based on that, we cannot determine if it is a good spill - // candidate or not. Skip processing it. - continue; - } - if ((recentRefPosition != nullptr) && linearScan->isRefPositionActive(recentRefPosition, thisLocation) && (recentRefPosition->needsConsecutive)) { continue; } -#endif // TARGET_ARM64 - - if ((linearScan->getNextIntervalRef(spillCandidateRegNum, regType) == thisLocation) && - !assignedInterval->getNextRefPosition()->RegOptional()) - { - continue; - } - if (!linearScan->isSpillCandidate(currentInterval, refPosition, spillCandidateRegRecord)) + else if (assignedInterval != nullptr) +#endif { - continue; - } + if ((linearScan->getNextIntervalRef(spillCandidateRegNum, regType) == thisLocation) && + !assignedInterval->getNextRefPosition()->RegOptional()) + { + continue; + } + if (!linearScan->isSpillCandidate(currentInterval, refPosition, spillCandidateRegRecord)) + { + continue; + } - weight_t currentSpillWeight = 0; - if ((recentRefPosition != nullptr) && - (recentRefPosition->RegOptional() && !(assignedInterval->isLocalVar && recentRefPosition->IsActualRef()))) - { - // We do not "spillAfter" if previous (recent) refPosition was regOptional or if it - // is not an actual ref. In those cases, we will reload in future (next) refPosition. - // For such cases, consider the spill cost of next refposition. - // See notes in "spillInterval()". - RefPosition* reloadRefPosition = assignedInterval->getNextRefPosition(); - if (reloadRefPosition != nullptr) + if ((recentRefPosition != nullptr) && (recentRefPosition->RegOptional() && + !(assignedInterval->isLocalVar && recentRefPosition->IsActualRef()))) { - currentSpillWeight = linearScan->getWeight(reloadRefPosition); + // We do not "spillAfter" if previous (recent) refPosition was regOptional or if it + // is not an actual ref. In those cases, we will reload in future (next) refPosition. + // For such cases, consider the spill cost of next refposition. + // See notes in "spillInterval()". + RefPosition* reloadRefPosition = assignedInterval->getNextRefPosition(); + if (reloadRefPosition != nullptr) + { + currentSpillWeight = linearScan->getWeight(reloadRefPosition); + } } } +#ifdef TARGET_ARM64 + else + { + // Ideally we should not be seeing this candidate because it is not assigned to + // any interval. But it is possible for certain scenarios. One of them is that + // `refPosition` needs consecutive registers and we decided to pick a mix of free+busy + // registers. This candidate is part of that set and is free and hence is not assigned + // to any interval. + } +#endif // TARGET_ARM64 // Only consider spillCost if we were not able to calculate weight of reloadRefPosition. if (currentSpillWeight == 0) @@ -11875,7 +11879,16 @@ void LinearScan::RegisterSelection::try_PREV_REG_OPT() #ifdef DEBUG // The assigned should be non-null, and should have a recentRefPosition, however since // this is a heuristic, we don't want a fatal error, so we just assert (not noway_assert). - if (!hasAssignedInterval) + if (!hasAssignedInterval +#ifdef TARGET_ARM64 + // We could see a candidate that doesn't have assignedInterval because allocation is + // happening for `refPosition` that needs consecutive registers and we decided to pick + // a mix of free+busy registers. This candidate is part of that set and is free and hence + // is not assigned to any interval. + + && !refPosition->needsConsecutive +#endif + ) { assert(!"Spill candidate has no assignedInterval recentRefPosition"); } From 17089648dec9407a81def95ff27dad911f58a533 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:40:09 -0700 Subject: [PATCH 02/11] Do not let limitRegs reduce the number of candidates If we find out that there are no candidates free/busy for refPositions that need consecutive registers, have at least one range of registers in the candidates such that allocation is possible. --- src/coreclr/jit/lsra.cpp | 60 ++++++++++++++++++++++++++++++++++------ 1 file changed, 51 insertions(+), 9 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 6703e5db764d54..9a1e4e4f5df38d 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -12199,6 +12199,10 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, } } +#ifdef DEBUG + regMaskTP inUseOrBusyRegsMask = RBM_NONE; +#endif + // Eliminate candidates that are in-use or busy. if (!found) { @@ -12208,6 +12212,10 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, regMaskTP busyRegs = linearScan->regsBusyUntilKill | linearScan->regsInUseThisLocation; candidates &= ~busyRegs; +#ifdef DEBUG + inUseOrBusyRegsMask |= ~busyRegs; +#endif + // Also eliminate as busy any register with a conflicting fixed reference at this or // the next location. // Note that this will eliminate the fixedReg, if any, but we'll add it back below. @@ -12223,6 +12231,9 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, (refPosition->delayRegFree && (checkConflictLocation == (refPosition->nodeLocation + 1)))) { candidates &= ~checkConflictBit; +#ifdef DEBUG + inUseOrBusyRegsMask |= ~checkConflictBit; +#endif } } candidates |= fixedRegMask; @@ -12258,13 +12269,6 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, prevRegBit = RBM_NONE; } - if (!found && (candidates == RBM_NONE)) - { - assert(refPosition->RegOptional()); - currentInterval->assignedReg = nullptr; - return RBM_NONE; - } - // TODO-Cleanup: Previously, the "reverseSelect" stress mode reversed the order of the heuristics. // It needs to be re-engineered with this refactoring. // In non-debug builds, this will simply get optimized away @@ -12273,9 +12277,9 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, reverseSelect = linearScan->doReverseSelect(); #endif // DEBUG -#ifdef TARGET_ARM64 if (needsConsecutiveRegisters) { +#ifdef TARGET_ARM64 regMaskTP busyConsecutiveCandidates = RBM_NONE; if (refPosition->isFirstRefPositionOfConsecutiveRegisters()) { @@ -12299,13 +12303,51 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, } if ((freeCandidates == RBM_NONE) && (candidates == RBM_NONE)) + { +#ifdef DEBUG + // Need to make sure that candidates has N consecutive registers to assign + if (linearScan->getStressLimitRegs() != LSRA_LIMIT_NONE) + { + // If the refPosition needs consecutive registers, then we want to make sure that + // the candidates have atleast one range of N registers that are consecutive, where N + // is the number of consecutive registers needed. + // Remove the `inUseOrBusyRegsMask` from the original candidates list and find one + // such range that is consecutive. Next, append that range to the `candidates`. + // + regMaskTP limitCandidatesForConsecutive = refPosition->registerAssignment & inUseOrBusyRegsMask; + regMaskTP overallLimitCandidates; + regMaskTP limitConsecutiveResult = + linearScan->filterConsecutiveCandidates(limitCandidatesForConsecutive, refPosition->regCount, + &overallLimitCandidates); + assert(limitConsecutiveResult != RBM_NONE); + + DWORD startRegister = 0; + BitScanForward64(&startRegister, static_cast(limitConsecutiveResult)); + + regMaskTP registersNeededMask = (1ULL << refPosition->regCount) - 1; + candidates |= (registersNeededMask << startRegister); + } + + if (candidates == RBM_NONE) +#endif // DEBUG + + { noway_assert(!"Not sufficient consecutive registers available."); } } - else #endif // TARGET_ARM64 + } + else { + + if (!found && (candidates == RBM_NONE)) + { + assert(refPosition->RegOptional()); + currentInterval->assignedReg = nullptr; + return RBM_NONE; + } + freeCandidates = linearScan->getFreeCandidates(candidates ARM_ARG(regType)); } From c31cfbdac27f0846d86611dca35e646f085688e0 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:41:56 -0700 Subject: [PATCH 03/11] getConsecutiveCandidates update if freeCandidate=RBM_NONE Intially, we were just returning RBM_NONE if we don't find any freeCandidates, but instead should try if we can find out if there are any busy candidates that we should try them out. --- src/coreclr/jit/lsraarm64.cpp | 103 ++++++++++++++++------------------ 1 file changed, 49 insertions(+), 54 deletions(-) diff --git a/src/coreclr/jit/lsraarm64.cpp b/src/coreclr/jit/lsraarm64.cpp index 72047f67a56b35..fe4a277c7c4830 100644 --- a/src/coreclr/jit/lsraarm64.cpp +++ b/src/coreclr/jit/lsraarm64.cpp @@ -198,11 +198,6 @@ regMaskTP LinearScan::filterConsecutiveCandidates(regMaskTP candidates, DWORD regAvailableStartIndex = 0, regAvailableEndIndex = 0; - // If we don't find consecutive registers, also track which registers we can pick so - // as to reduce the number of registers we will have to spill, to accomodate the - // request of the consecutive registers. - regMaskTP registersNeededMask = (1ULL << registersNeeded) - 1; - do { // From LSB, find the first available register (bit `1`) @@ -421,77 +416,77 @@ regMaskTP LinearScan::getConsecutiveCandidates(regMaskTP allCandidates, assert(compiler->info.compNeedsConsecutiveRegisters); assert(refPosition->isFirstRefPositionOfConsecutiveRegisters()); regMaskTP freeCandidates = allCandidates & m_AvailableRegs; - if (freeCandidates == RBM_NONE) - { - return freeCandidates; - } *busyCandidates = RBM_NONE; regMaskTP overallResult; unsigned int registersNeeded = refPosition->regCount; - regMaskTP consecutiveResultForFree = filterConsecutiveCandidates(freeCandidates, registersNeeded, &overallResult); - if (consecutiveResultForFree != RBM_NONE) + if (freeCandidates != RBM_NONE) { - // One last time, check if subsequent RefPositions (all RefPositions except the first for which - // we assigned above) already have consecutive registers assigned. If yes, and if one of the - // register out of the `consecutiveResult` is available for the first RefPosition, then just use - // that. This will avoid unnecessary copies. + regMaskTP consecutiveResultForFree = + filterConsecutiveCandidates(freeCandidates, registersNeeded, &overallResult); - regNumber firstRegNum = REG_NA; - regNumber prevRegNum = REG_NA; - int foundCount = 0; - regMaskTP foundRegMask = RBM_NONE; + if (consecutiveResultForFree != RBM_NONE) + { + // One last time, check if subsequent RefPositions (all RefPositions except the first for which + // we assigned above) already have consecutive registers assigned. If yes, and if one of the + // register out of the `consecutiveResult` is available for the first RefPosition, then just use + // that. This will avoid unnecessary copies. - RefPosition* consecutiveRefPosition = getNextConsecutiveRefPosition(refPosition); - assert(consecutiveRefPosition != nullptr); + regNumber firstRegNum = REG_NA; + regNumber prevRegNum = REG_NA; + int foundCount = 0; + regMaskTP foundRegMask = RBM_NONE; - for (unsigned int i = 1; i < registersNeeded; i++) - { - Interval* interval = consecutiveRefPosition->getInterval(); - consecutiveRefPosition = getNextConsecutiveRefPosition(consecutiveRefPosition); + RefPosition* consecutiveRefPosition = getNextConsecutiveRefPosition(refPosition); + assert(consecutiveRefPosition != nullptr); - if (!interval->isActive) + for (unsigned int i = 1; i < registersNeeded; i++) { + Interval* interval = consecutiveRefPosition->getInterval(); + consecutiveRefPosition = getNextConsecutiveRefPosition(consecutiveRefPosition); + + if (!interval->isActive) + { + foundRegMask = RBM_NONE; + foundCount = 0; + continue; + } + + regNumber currRegNum = interval->assignedReg->regNum; + if ((prevRegNum == REG_NA) || (prevRegNum == REG_PREV(currRegNum)) || + ((prevRegNum == REG_FP_LAST) && (currRegNum == REG_FP_FIRST))) + { + foundRegMask |= genRegMask(currRegNum); + if (prevRegNum == REG_NA) + { + firstRegNum = currRegNum; + } + prevRegNum = currRegNum; + foundCount++; + continue; + } + foundRegMask = RBM_NONE; foundCount = 0; - continue; + break; } - regNumber currRegNum = interval->assignedReg->regNum; - if ((prevRegNum == REG_NA) || (prevRegNum == REG_PREV(currRegNum)) || - ((prevRegNum == REG_FP_LAST) && (currRegNum == REG_FP_FIRST))) + if (foundCount != 0) { - foundRegMask |= genRegMask(currRegNum); - if (prevRegNum == REG_NA) + assert(firstRegNum != REG_NA); + regMaskTP remainingRegsMask = ((1ULL << (registersNeeded - foundCount)) - 1) << (firstRegNum - 1); + + if ((overallResult & remainingRegsMask) != RBM_NONE) { - firstRegNum = currRegNum; + // If remaining registers are available, then just set the firstRegister mask + consecutiveResultForFree = 1ULL << (firstRegNum - 1); } - prevRegNum = currRegNum; - foundCount++; - continue; } - foundRegMask = RBM_NONE; - foundCount = 0; - break; - } - - if (foundCount != 0) - { - assert(firstRegNum != REG_NA); - regMaskTP remainingRegsMask = ((1ULL << (registersNeeded - foundCount)) - 1) << (firstRegNum - 1); - - if ((overallResult & remainingRegsMask) != RBM_NONE) - { - // If remaining registers are available, then just set the firstRegister mask - consecutiveResultForFree = 1ULL << (firstRegNum - 1); - } + return consecutiveResultForFree; } - - return consecutiveResultForFree; } - // There are registers available but they are not consecutive. // Here are some options to address them: // From e684275ee0eee8324e6c717c98de776021b519c9 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:45:07 -0700 Subject: [PATCH 04/11] Relax limitStressRegs for refpositions live at consecutive register position If consecutive registers are being allocated, other refpositions that are live at the same location might not have enough registers left to be assigned because all registers are busy. As such, introduce a way to track if we are assigning at the location of consecutive registers, and if yes, do not take jitstressregs limit into account. --- src/coreclr/jit/lsra.cpp | 32 ++++++++++++++++++++++++++++++++ src/coreclr/jit/lsra.h | 6 +++++- 2 files changed, 37 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 9a1e4e4f5df38d..010a39d2c2fd32 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -663,6 +663,8 @@ LinearScan::LinearScan(Compiler* theCompiler) #ifdef DEBUG maxNodeLocation = 0; + consecutiveRegistersLocation = 0; + activeRefPosition = nullptr; currBuildNode = nullptr; @@ -4901,6 +4903,24 @@ void LinearScan::allocateRegisters() } } prevLocation = currentLocation; +#ifdef TARGET_ARM64 + +#ifdef DEBUG + if (hasConsecutiveRegister) + { + if (currentRefPosition.needsConsecutive) + { + // track all the refpositions around the location that is also + // allocating consecutive registers. + consecutiveRegistersLocation = currentLocation; + } + else if (consecutiveRegistersLocation < currentLocation) + { + consecutiveRegistersLocation = MinLocation; + } + } +#endif // DEBUG +#endif // TARGET_ARM64 // get previous refposition, then current refpos is the new previous if (currentReferent != nullptr) @@ -12049,7 +12069,19 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, } #ifdef DEBUG +#ifdef TARGET_ARM64 + if (!refPosition->needsConsecutive && (linearScan->consecutiveRegistersLocation == refPosition->nodeLocation)) + { + // If a method has consecutive registers and we are assigning to refPositions that are not part + // of consecutive registers, but are live at same location, skip the limit stress for them, because + // there are high chances that many registers are busy for consecutive requirements and we don't + // have enough remaining for other refpositions (like operands). + } + else +#endif + { candidates = linearScan->stressLimitRegs(refPosition, candidates); + } #endif assert(candidates != RBM_NONE); diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index bf72219e755c2d..0702d5ba0d5304 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -2006,9 +2006,13 @@ class LinearScan : public LinearScanInterface int BuildHWIntrinsic(GenTreeHWIntrinsic* intrinsicTree, int* pDstCount); #ifdef TARGET_ARM64 int BuildConsecutiveRegistersForUse(GenTree* treeNode, GenTree* rmwNode = nullptr); -#endif +#endif // TARGET_ARM64 #endif // FEATURE_HW_INTRINSICS +#ifdef DEBUG + LsraLocation consecutiveRegistersLocation; +#endif // DEBUG + int BuildPutArgStk(GenTreePutArgStk* argNode); #if FEATURE_ARG_SPLIT int BuildPutArgSplit(GenTreePutArgSplit* tree); From 13e85ac0f7638a2da7ae9ceb5871652430ba64fe Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:46:45 -0700 Subject: [PATCH 05/11] Update minRegCount for registerAssignment For consecutive register, also include the register count needed for "minimum register requirement" when limiting the registers. --- src/coreclr/jit/lsrabuild.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 006b948626a60e..87d5290b601199 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -1805,6 +1805,13 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, LsraLocation currentLoc { minRegCount++; } +#ifdef TARGET_ARM64 + else if (newRefPosition->needsConsecutive) + { + assert(newRefPosition->refType == RefTypeUpperVectorRestore); + minRegCount++; + } +#endif #endif if (newRefPosition->getInterval()->isSpecialPutArg) { @@ -1859,12 +1866,7 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, LsraLocation currentLoc regMaskTP calleeSaveMask = calleeSaveRegs(interval->registerType); newRefPosition->registerAssignment = getConstrainedRegMask(oldAssignment, calleeSaveMask, minRegCountForRef); -#ifdef TARGET_ARM64 - if (newRefPosition->isFirstRefPositionOfConsecutiveRegisters()) - { - newRefPosition->registerAssignment |= LsraLimitFPSetForConsecutive; - } -#endif + if ((newRefPosition->registerAssignment != oldAssignment) && (newRefPosition->refType == RefTypeUse) && !interval->isLocalVar) { From f2cda1e51c74d54bf23f714979450e8c9cd13b29 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:47:54 -0700 Subject: [PATCH 06/11] Remove LsraLimitFPSetForConsecutive With other conditions in place, no need to have LsraLimitFPSetForConsecutive. --- src/coreclr/jit/lsra.cpp | 7 ------- src/coreclr/jit/lsra.h | 3 --- 2 files changed, 10 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 010a39d2c2fd32..da73f8798adb02 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -496,13 +496,6 @@ regMaskTP LinearScan::stressLimitRegs(RefPosition* refPosition, regMaskTP mask) { mask |= refPosition->registerAssignment; } - -#ifdef TARGET_ARM64 - if ((refPosition != nullptr) && refPosition->isFirstRefPositionOfConsecutiveRegisters()) - { - mask |= LsraLimitFPSetForConsecutive; - } -#endif } return mask; diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index 0702d5ba0d5304..0aef405f35a9f9 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -792,9 +792,6 @@ class LinearScan : public LinearScanInterface #elif defined(TARGET_ARM64) static const regMaskTP LsraLimitSmallIntSet = (RBM_R0 | RBM_R1 | RBM_R2 | RBM_R19 | RBM_R20); static const regMaskTP LsraLimitSmallFPSet = (RBM_V0 | RBM_V1 | RBM_V2 | RBM_V8 | RBM_V9); - // LsraLimitFPSetForConsecutive is used for stress mode and gives few extra registers to satisfy - // the requirements for allocating consecutive registers. - static const regMaskTP LsraLimitFPSetForConsecutive = (RBM_V3 | RBM_V5 | RBM_V7); #elif defined(TARGET_X86) static const regMaskTP LsraLimitSmallIntSet = (RBM_EAX | RBM_ECX | RBM_EDI); static const regMaskTP LsraLimitSmallFPSet = (RBM_XMM0 | RBM_XMM1 | RBM_XMM2 | RBM_XMM6 | RBM_XMM7); From c0c3f4a7a0ab12d865b9225df781fe1ef8a6d955 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:48:47 -0700 Subject: [PATCH 07/11] Added an assert --- src/coreclr/jit/lsra.cpp | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index da73f8798adb02..d82cd2cd6151b1 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -12014,6 +12014,10 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, *registerScore = NONE; #endif +#ifdef TARGET_ARM64 + assert(!needsConsecutiveRegisters || refPosition->needsConsecutive); +#endif + reset(currentInterval, refPosition); // process data-structures From 01ec812845bbefc05bbf70c5dae4737a4d5eeb6b Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 15:52:12 -0700 Subject: [PATCH 08/11] misc changes --- src/coreclr/jit/lsra.cpp | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index d82cd2cd6151b1..9289267082d175 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -12077,7 +12077,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, else #endif { - candidates = linearScan->stressLimitRegs(refPosition, candidates); + candidates = linearScan->stressLimitRegs(refPosition, candidates); } #endif assert(candidates != RBM_NONE); @@ -12279,12 +12279,10 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, prevRegBit = genRegMask(prevRegRec->regNum); if ((prevRegRec->assignedInterval == currentInterval) && ((candidates & prevRegBit) != RBM_NONE)) { -#ifdef TARGET_ARM64 - // If this is allocating for consecutive register, we need to make sure that - // we allocate register, whose consecutive registers are also free. if (!needsConsecutiveRegisters) -#endif { + // If this is allocating for consecutive register, we need to make sure that + // we allocate register, whose consecutive registers are also free. candidates = prevRegBit; found = true; #ifdef DEBUG @@ -12333,7 +12331,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, if ((freeCandidates == RBM_NONE) && (candidates == RBM_NONE)) { -#ifdef DEBUG +#ifdef DEBUG // Need to make sure that candidates has N consecutive registers to assign if (linearScan->getStressLimitRegs() != LSRA_LIMIT_NONE) { @@ -12358,18 +12356,15 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, } if (candidates == RBM_NONE) -#endif // DEBUG - - - { - noway_assert(!"Not sufficient consecutive registers available."); +#endif // DEBUG + { + noway_assert(!"Not sufficient consecutive registers available."); + } } - } #endif // TARGET_ARM64 } else { - if (!found && (candidates == RBM_NONE)) { assert(refPosition->RegOptional()); From e5698ccebce1d7be859d2465a43e37574f81e555 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 21:25:01 -0700 Subject: [PATCH 09/11] jit format --- src/coreclr/jit/lsra.cpp | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 9289267082d175..3d6d0b2458270a 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -655,7 +655,7 @@ LinearScan::LinearScan(Compiler* theCompiler) firstColdLoc = MaxLocation; #ifdef DEBUG - maxNodeLocation = 0; + maxNodeLocation = 0; consecutiveRegistersLocation = 0; activeRefPosition = nullptr; @@ -11696,8 +11696,8 @@ void LinearScan::RegisterSelection::try_SPILL_COST() Interval* assignedInterval = spillCandidateRegRecord->assignedInterval; RefPosition* recentRefPosition = assignedInterval != nullptr ? assignedInterval->recentRefPosition : nullptr; -// Can and should the interval in this register be spilled for this one, -// if we don't find a better alternative? + // Can and should the interval in this register be spilled for this one, + // if we don't find a better alternative? weight_t currentSpillWeight = 0; #ifdef TARGET_ARM64 @@ -12072,7 +12072,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, // If a method has consecutive registers and we are assigning to refPositions that are not part // of consecutive registers, but are live at same location, skip the limit stress for them, because // there are high chances that many registers are busy for consecutive requirements and we don't - // have enough remaining for other refpositions (like operands). + // have enough remaining for other refpositions (like operands). } else #endif @@ -12340,7 +12340,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, // is the number of consecutive registers needed. // Remove the `inUseOrBusyRegsMask` from the original candidates list and find one // such range that is consecutive. Next, append that range to the `candidates`. - // + // regMaskTP limitCandidatesForConsecutive = refPosition->registerAssignment & inUseOrBusyRegsMask; regMaskTP overallLimitCandidates; regMaskTP limitConsecutiveResult = From 2a3d0f2e7841fbe4fcf9e7496bc23bfb0841fd97 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Mon, 10 Apr 2023 22:00:51 -0700 Subject: [PATCH 10/11] Use BitOperations::BitScanForward() --- src/coreclr/jit/lsra.cpp | 3 +-- src/coreclr/jit/lsraarm64.cpp | 12 ++++++------ 2 files changed, 7 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 3d6d0b2458270a..a8adb7d66a0b5c 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -12348,8 +12348,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, &overallLimitCandidates); assert(limitConsecutiveResult != RBM_NONE); - DWORD startRegister = 0; - BitScanForward64(&startRegister, static_cast(limitConsecutiveResult)); + unsigned startRegister = BitOperations::BitScanForward(limitConsecutiveResult); regMaskTP registersNeededMask = (1ULL << refPosition->regCount) - 1; candidates |= (registersNeededMask << startRegister); diff --git a/src/coreclr/jit/lsraarm64.cpp b/src/coreclr/jit/lsraarm64.cpp index fe4a277c7c4830..5e0385b7fc1b20 100644 --- a/src/coreclr/jit/lsraarm64.cpp +++ b/src/coreclr/jit/lsraarm64.cpp @@ -196,13 +196,13 @@ regMaskTP LinearScan::filterConsecutiveCandidates(regMaskTP candidates, consecutiveResult |= availableRegistersMask & (selectionEndMask & ~selectionStartMask); \ overallResult |= availableRegistersMask; - DWORD regAvailableStartIndex = 0, regAvailableEndIndex = 0; + unsigned regAvailableStartIndex = 0, regAvailableEndIndex = 0; do { // From LSB, find the first available register (bit `1`) - BitScanForward64(®AvailableStartIndex, static_cast(currAvailableRegs)); - regMaskTP startMask = (1ULL << regAvailableStartIndex) - 1; + regAvailableStartIndex = BitOperations::BitScanForward(static_cast(currAvailableRegs)); + regMaskTP startMask = (1ULL << regAvailableStartIndex) - 1; // Mask all the bits that are processed from LSB thru regAvailableStart until the last `1`. regMaskTP maskProcessed = ~(currAvailableRegs | startMask); @@ -219,7 +219,7 @@ regMaskTP LinearScan::filterConsecutiveCandidates(regMaskTP candidates, } else { - BitScanForward64(®AvailableEndIndex, static_cast(maskProcessed)); + regAvailableEndIndex = BitOperations::BitScanForward(static_cast(maskProcessed)); } regMaskTP endMask = (1ULL << regAvailableEndIndex) - 1; @@ -325,13 +325,13 @@ regMaskTP LinearScan::filterConsecutiveCandidatesForSpill(regMaskTP consecutiveC assert((registersNeeded >= 2) && (registersNeeded <= 4)); regMaskTP consecutiveResultForBusy = RBM_NONE; regMaskTP unprocessedRegs = consecutiveCandidates; - DWORD regAvailableStartIndex = 0, regAvailableEndIndex = 0; + unsigned regAvailableStartIndex = 0, regAvailableEndIndex = 0; int maxSpillRegs = registersNeeded; regMaskTP registersNeededMask = (1ULL << registersNeeded) - 1; do { // From LSB, find the first available register (bit `1`) - BitScanForward64(®AvailableStartIndex, static_cast(unprocessedRegs)); + regAvailableStartIndex = BitOperations::BitScanForward(static_cast(unprocessedRegs)); // For the current range, find how many registers are free vs. busy regMaskTP maskForCurRange = RBM_NONE; From 3f5dfd7bd06432cabc2a526e5a2112f0b2547aa2 Mon Sep 17 00:00:00 2001 From: Kunal Pathak Date: Tue, 11 Apr 2023 07:01:54 -0700 Subject: [PATCH 11/11] review feedback --- src/coreclr/jit/lsra.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index a8adb7d66a0b5c..68d9c3baa163df 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -12242,7 +12242,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, candidates &= ~busyRegs; #ifdef DEBUG - inUseOrBusyRegsMask |= ~busyRegs; + inUseOrBusyRegsMask |= busyRegs; #endif // Also eliminate as busy any register with a conflicting fixed reference at this or @@ -12261,7 +12261,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, { candidates &= ~checkConflictBit; #ifdef DEBUG - inUseOrBusyRegsMask |= ~checkConflictBit; + inUseOrBusyRegsMask |= checkConflictBit; #endif } } @@ -12341,7 +12341,7 @@ regMaskTP LinearScan::RegisterSelection::select(Interval* currentInterval, // Remove the `inUseOrBusyRegsMask` from the original candidates list and find one // such range that is consecutive. Next, append that range to the `candidates`. // - regMaskTP limitCandidatesForConsecutive = refPosition->registerAssignment & inUseOrBusyRegsMask; + regMaskTP limitCandidatesForConsecutive = refPosition->registerAssignment & ~inUseOrBusyRegsMask; regMaskTP overallLimitCandidates; regMaskTP limitConsecutiveResult = linearScan->filterConsecutiveCandidates(limitCandidatesForConsecutive, refPosition->regCount,