From 13bc9d4283554a0f31afa35a2152a54c5d291a50 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 5 Oct 2022 17:00:27 +0200 Subject: [PATCH 1/8] JIT: Preference locals away from PUTARG_REG killed registers --- src/coreclr/jit/lsrabuild.cpp | 46 +++++++++++++++++++++++++++++++++++ 1 file changed, 46 insertions(+) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 370bfc393f23e..7bab46b39d863 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -3917,6 +3917,52 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) def->getInterval()->assignRelatedInterval(use->getInterval()); } } + + if (enregisterLocalVars) + { + // Preference locals that are still live out of this register. + // If they are used before the call and we allocate it to this register it would force a spill. + // If they are used after the call, then the preference set is anyway + // going to become the callee-saved registers when we see the call which + // in the common case does not conflict with the argument registers. + + // Note that if 'op1' is a local we should not unpreference it from the + // register since PUTARG_REG is pass through. We know how to use the + // value out of this register between the PUTARG_REG and the call when + // "special putarg" is supported. + unsigned skipVarIndex = UINT_MAX; + if (op1->IsLocal()) + { + LclVarDsc* op1Lcl = compiler->lvaGetDesc(op1->AsLclVarCommon()); + if (op1Lcl->lvTracked) + { + skipVarIndex = op1Lcl->lvVarIndex; + } + } + + VarSetOps::Iter iter(compiler, currentLiveVars); + unsigned varIndex = 0; + while (iter.NextElem(&varIndex)) + { + if (varIndex == skipVarIndex) + { + continue; + } + + Interval* interval = getIntervalForLocalVar(varIndex); + // Spilling a write-thru register is free so avoid preferencing it + // away from these registers. + if (interval->isWriteThru) + { + continue; + } + + regMaskTP newPreferences = allRegs(interval->registerType) & ~argMask; + assert(newPreferences != RBM_NONE); + interval->updateRegisterPreferences(newPreferences); + } + } + return srcCount; } From b2ce3f713e06f42c9d12ded595b6b4ee0e6d5380 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 5 Oct 2022 17:10:56 +0200 Subject: [PATCH 2/8] Nit --- src/coreclr/jit/lsrabuild.cpp | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 7bab46b39d863..807bdb53b0939 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -3918,15 +3918,16 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) } } + // Preference locals that are still live out of this register. + // If they are used before the call and we allocate it to this register + // it would force a spill. + // If they are used after the call, then the preference set is anyway + // going to become the callee-saved registers when we see the call + // which in the common case does not conflict with the argument + // registers. if (enregisterLocalVars) { - // Preference locals that are still live out of this register. - // If they are used before the call and we allocate it to this register it would force a spill. - // If they are used after the call, then the preference set is anyway - // going to become the callee-saved registers when we see the call which - // in the common case does not conflict with the argument registers. - - // Note that if 'op1' is a local we should not unpreference it from the + // If 'op1' is a local we should not unpreference it from the // register since PUTARG_REG is pass through. We know how to use the // value out of this register between the PUTARG_REG and the call when // "special putarg" is supported. @@ -3950,7 +3951,7 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) } Interval* interval = getIntervalForLocalVar(varIndex); - // Spilling a write-thru register is free so avoid preferencing it + // Spilling a write-thru local is free so avoid preferencing it // away from these registers. if (interval->isWriteThru) { From bf5113eba83fe1658905bdf1ac7cfaa1f131b835 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 5 Oct 2022 23:19:45 +0200 Subject: [PATCH 3/8] Improve and optimize throughput --- src/coreclr/jit/lsra.h | 20 ++++++ src/coreclr/jit/lsraarmarch.cpp | 4 ++ src/coreclr/jit/lsrabuild.cpp | 110 +++++++++++++++++++------------- src/coreclr/jit/lsraxarch.cpp | 4 ++ 4 files changed, 95 insertions(+), 43 deletions(-) diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index 5dbdd4afd8570..8cb6bd4062ef1 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -1586,6 +1586,25 @@ class LinearScan : public LinearScanInterface PhasedVar availableFloatRegs; PhasedVar availableDoubleRegs; + // Register mask of argument registers currently occupied because we saw a + // PUTARG_REG node. Tracked between the PUTARG_REG and its corresponding + // CALL node and used to preference locals away from these (occupied) + // registers that will otherwise force a spill. + regMaskTP placedArgRegs; + + struct PlacedLocal + { + unsigned VarIndex; + regMaskTP RegMask; + }; + + // Locals that are currently placed in registers via PUTARG_REG. These + // locals are available due to the special PUTARG treatment, and we keep + // track of them between the PUTARG_REG and CALL to ensure we do not + // unpreference them. + PlacedLocal placedArgLocals[MAX_REG_ARG + MAX_FLOAT_REG_ARG]; + size_t numPlacedArgLocals; + // The set of all register candidates. Note that this may be a subset of tracked vars. VARSET_TP registerCandidateVars; // Current set of live register candidate vars, used during building of RefPositions to determine @@ -1823,6 +1842,7 @@ class LinearScan : public LinearScanInterface // These methods return the number of sources. int BuildNode(GenTree* tree); + void PreferenceDyingLocal(Interval* interval); void getTgtPrefOperands(GenTree* tree, GenTree* op1, GenTree* op2, bool* prefOp1, bool* prefOp2); bool supportsSpecialPutArg(); diff --git a/src/coreclr/jit/lsraarmarch.cpp b/src/coreclr/jit/lsraarmarch.cpp index d41eef6ad2ed5..2c8a16af1a864 100644 --- a/src/coreclr/jit/lsraarmarch.cpp +++ b/src/coreclr/jit/lsraarmarch.cpp @@ -384,6 +384,10 @@ int LinearScan::BuildCall(GenTreeCall* call) // Now generate defs and kills. regMaskTP killMask = getKillSetForCall(call); BuildDefsWithKills(call, dstCount, dstCandidates, killMask); + + // No args are placed in registers anymore. + placedArgRegs = RBM_NONE; + numPlacedArgLocals = 0; return srcCount; } diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 807bdb53b0939..b234334449ed9 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -1734,6 +1734,8 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, LsraLocation currentLoc assert(varDsc->lvTracked); unsigned varIndex = varDsc->lvVarIndex; VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex); + + PreferenceDyingLocal(getIntervalForLocalVar(varIndex)); } } #else // TARGET_XARCH @@ -2260,6 +2262,9 @@ void LinearScan::buildIntervals() intRegState->rsCalleeRegArgMaskLiveIn |= RBM_SECRET_STUB_PARAM; } + numPlacedArgLocals = 0; + placedArgRegs = RBM_NONE; + BasicBlock* predBlock = nullptr; BasicBlock* prevBlock = nullptr; @@ -2946,6 +2951,58 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa BuildDefs(tree, dstCount, dstCandidates); } +//------------------------------------------------------------------------ +// PreferenceDyingLocal: Preference a local that is dying. +// +// Arguments: +// interval - the interval for the local +// +// Notes: +// The "dying" information here is approximate, see the comment in BuildUse. +// +void LinearScan::PreferenceDyingLocal(Interval* interval) +{ + if (placedArgRegs == RBM_NONE) + { + return; + } + // If we see a use of a local between placing a register and a call + // then we want to preference that local away from that particular + // argument register. Picking that register is otherwise going to force + // a spill. + // + // We only need to do this on liveness updates because if the local is live + // _after_ the call, then we are going to preference it to callee-saved + // registers anyway, so there is no need to look at such local uses. + + // Write-thru locals are "free" to spill and we are quite conservative + // about allocating them to callee-saved registers, so leave them alone + // here. + if (interval->isWriteThru) + { + return; + } + + // Find registers that are occupied with other values. + regMaskTP unpref = placedArgRegs; + unsigned varIndex = interval->getVarIndex(compiler); + for (int i = 0; i < numPlacedArgLocals; i++) + { + if (placedArgLocals[i].VarIndex == varIndex) + { + // This local's value is going to be available in this register so + // keep it in the preferences. + unpref &= ~placedArgLocals[i].RegMask; + } + } + + regMaskTP newPreferences = allRegs(interval->registerType) & ~unpref; + if (newPreferences != RBM_NONE) + { + interval->updateRegisterPreferences(newPreferences); + } +} + //------------------------------------------------------------------------ // BuildUse: Remove the RefInfoListNode for the given multi-reg index of the given node from // the defList, and build a use RefPosition for the associated Interval. @@ -2985,6 +3042,7 @@ RefPosition* LinearScan::BuildUse(GenTree* operand, regMaskTP candidates, int mu { unsigned varIndex = interval->getVarIndex(compiler); VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex); + PreferenceDyingLocal(interval); } #if FEATURE_PARTIAL_SIMD_CALLEE_SAVE buildUpperVectorRestoreRefPosition(interval, currentLoc, operand, true); @@ -3918,50 +3976,16 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) } } - // Preference locals that are still live out of this register. - // If they are used before the call and we allocate it to this register - // it would force a spill. - // If they are used after the call, then the preference set is anyway - // going to become the callee-saved registers when we see the call - // which in the common case does not conflict with the argument - // registers. - if (enregisterLocalVars) - { - // If 'op1' is a local we should not unpreference it from the - // register since PUTARG_REG is pass through. We know how to use the - // value out of this register between the PUTARG_REG and the call when - // "special putarg" is supported. - unsigned skipVarIndex = UINT_MAX; - if (op1->IsLocal()) - { - LclVarDsc* op1Lcl = compiler->lvaGetDesc(op1->AsLclVarCommon()); - if (op1Lcl->lvTracked) - { - skipVarIndex = op1Lcl->lvVarIndex; - } - } + placedArgRegs |= argMask; - VarSetOps::Iter iter(compiler, currentLiveVars); - unsigned varIndex = 0; - while (iter.NextElem(&varIndex)) - { - if (varIndex == skipVarIndex) - { - continue; - } - - Interval* interval = getIntervalForLocalVar(varIndex); - // Spilling a write-thru local is free so avoid preferencing it - // away from these registers. - if (interval->isWriteThru) - { - continue; - } - - regMaskTP newPreferences = allRegs(interval->registerType) & ~argMask; - assert(newPreferences != RBM_NONE); - interval->updateRegisterPreferences(newPreferences); - } + // If this is a passthrough tracked local then record it so that we can + // ensure we do not unpreference if we see a future use (see OnLocalDying). + if (isSpecialPutArg) + { + assert(numPlacedArgLocals < ArrLen(placedArgLocals)); + placedArgLocals[numPlacedArgLocals].VarIndex = use->getInterval()->getVarIndex(compiler); + placedArgLocals[numPlacedArgLocals].RegMask = argMask; + numPlacedArgLocals++; } return srcCount; diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp index 3c5220ba1d914..e248620825a9c 100644 --- a/src/coreclr/jit/lsraxarch.cpp +++ b/src/coreclr/jit/lsraxarch.cpp @@ -1238,6 +1238,10 @@ int LinearScan::BuildCall(GenTreeCall* call) // Now generate defs and kills. regMaskTP killMask = getKillSetForCall(call); BuildDefsWithKills(call, dstCount, dstCandidates, killMask); + + // No args are placed in registers anymore. + placedArgRegs = RBM_NONE; + numPlacedArgLocals = 0; return srcCount; } From ae4a4e24fb9d673d66dd98bb24b793961a721b68 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 5 Oct 2022 23:37:43 +0200 Subject: [PATCH 4/8] Fix build --- src/coreclr/jit/lsrabuild.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index b234334449ed9..06de636fadf51 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -2986,7 +2986,7 @@ void LinearScan::PreferenceDyingLocal(Interval* interval) // Find registers that are occupied with other values. regMaskTP unpref = placedArgRegs; unsigned varIndex = interval->getVarIndex(compiler); - for (int i = 0; i < numPlacedArgLocals; i++) + for (size_t i = 0; i < numPlacedArgLocals; i++) { if (placedArgLocals[i].VarIndex == varIndex) { From a3f56447ce02c56993403c2c89ad0b3b08db073a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 5 Oct 2022 23:49:45 +0200 Subject: [PATCH 5/8] Keep it simple and obviously correct by using REG_COUNT --- src/coreclr/jit/lsra.h | 4 ++-- src/coreclr/jit/lsrabuild.cpp | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index 8cb6bd4062ef1..65aa293d19ca2 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -1595,14 +1595,14 @@ class LinearScan : public LinearScanInterface struct PlacedLocal { unsigned VarIndex; - regMaskTP RegMask; + regNumber Reg; }; // Locals that are currently placed in registers via PUTARG_REG. These // locals are available due to the special PUTARG treatment, and we keep // track of them between the PUTARG_REG and CALL to ensure we do not // unpreference them. - PlacedLocal placedArgLocals[MAX_REG_ARG + MAX_FLOAT_REG_ARG]; + PlacedLocal placedArgLocals[REG_COUNT]; size_t numPlacedArgLocals; // The set of all register candidates. Note that this may be a subset of tracked vars. diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 06de636fadf51..1788e7039a70c 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -2992,7 +2992,7 @@ void LinearScan::PreferenceDyingLocal(Interval* interval) { // This local's value is going to be available in this register so // keep it in the preferences. - unpref &= ~placedArgLocals[i].RegMask; + unpref &= ~genRegMask(placedArgLocals[i].Reg); } } @@ -3979,12 +3979,12 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) placedArgRegs |= argMask; // If this is a passthrough tracked local then record it so that we can - // ensure we do not unpreference if we see a future use (see OnLocalDying). + // ensure we do not unpreference if we see a future use (see PreferenceDyingLocal). if (isSpecialPutArg) { assert(numPlacedArgLocals < ArrLen(placedArgLocals)); placedArgLocals[numPlacedArgLocals].VarIndex = use->getInterval()->getVarIndex(compiler); - placedArgLocals[numPlacedArgLocals].RegMask = argMask; + placedArgLocals[numPlacedArgLocals].Reg = argReg; numPlacedArgLocals++; } From 8815d2a061b7e431700775d020b2e75b25d723e3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 6 Oct 2022 21:49:55 +0200 Subject: [PATCH 6/8] Address feedback --- src/coreclr/jit/lsra.h | 10 +++--- src/coreclr/jit/lsrabuild.cpp | 64 +++++++++++++++++++++-------------- 2 files changed, 44 insertions(+), 30 deletions(-) diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index 65aa293d19ca2..36bad01a6162d 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -1588,8 +1588,8 @@ class LinearScan : public LinearScanInterface // Register mask of argument registers currently occupied because we saw a // PUTARG_REG node. Tracked between the PUTARG_REG and its corresponding - // CALL node and used to preference locals away from these (occupied) - // registers that will otherwise force a spill. + // CALL node and is used to avoid preferring these registers for locals + // which would otherwise force a spill. regMaskTP placedArgRegs; struct PlacedLocal @@ -1600,8 +1600,8 @@ class LinearScan : public LinearScanInterface // Locals that are currently placed in registers via PUTARG_REG. These // locals are available due to the special PUTARG treatment, and we keep - // track of them between the PUTARG_REG and CALL to ensure we do not - // unpreference them. + // track of them between the PUTARG_REG and CALL to ensure we keep the + // register they are placed in in the preference set. PlacedLocal placedArgLocals[REG_COUNT]; size_t numPlacedArgLocals; @@ -1842,7 +1842,7 @@ class LinearScan : public LinearScanInterface // These methods return the number of sources. int BuildNode(GenTree* tree); - void PreferenceDyingLocal(Interval* interval); + void UpdatePreferenceOfDyingLocal(Interval* interval); void getTgtPrefOperands(GenTree* tree, GenTree* op1, GenTree* op2, bool* prefOp1, bool* prefOp2); bool supportsSpecialPutArg(); diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 1788e7039a70c..08157202fc2b6 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -1735,7 +1735,7 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, LsraLocation currentLoc unsigned varIndex = varDsc->lvVarIndex; VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex); - PreferenceDyingLocal(getIntervalForLocalVar(varIndex)); + UpdatePreferenceOfDyingLocal(getIntervalForLocalVar(varIndex)); } } #else // TARGET_XARCH @@ -2952,7 +2952,7 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa } //------------------------------------------------------------------------ -// PreferenceDyingLocal: Preference a local that is dying. +// UpdatePreferenceOfDyingLocal: Update the preference of a dying local. // // Arguments: // interval - the interval for the local @@ -2960,20 +2960,22 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa // Notes: // The "dying" information here is approximate, see the comment in BuildUse. // -void LinearScan::PreferenceDyingLocal(Interval* interval) +void LinearScan::UpdatePreferenceOfDyingLocal(Interval* interval) { + assert(!VarSetOps::IsMember(compiler, currentLiveVars, interval->getVarIndex(compiler))); + + // If we see a use of a local between placing a register and a call then we + // want to update that local's preferences to exclude that register. + // Picking that register is otherwise going to force a spill. + // + // We only need to do this on liveness updates because if the local is live + // _after_ the call, then we are going to prefer callee-saved registers for + // it anyway, so there is no need to look at such local uses. + // if (placedArgRegs == RBM_NONE) { return; } - // If we see a use of a local between placing a register and a call - // then we want to preference that local away from that particular - // argument register. Picking that register is otherwise going to force - // a spill. - // - // We only need to do this on liveness updates because if the local is live - // _after_ the call, then we are going to preference it to callee-saved - // registers anyway, so there is no need to look at such local uses. // Write-thru locals are "free" to spill and we are quite conservative // about allocating them to callee-saved registers, so leave them alone @@ -2983,7 +2985,8 @@ void LinearScan::PreferenceDyingLocal(Interval* interval) return; } - // Find registers that are occupied with other values. + // Find the registers that we should remove from the preference set because + // they are occupied with argument values. regMaskTP unpref = placedArgRegs; unsigned varIndex = interval->getVarIndex(compiler); for (size_t i = 0; i < numPlacedArgLocals; i++) @@ -2999,6 +3002,18 @@ void LinearScan::PreferenceDyingLocal(Interval* interval) regMaskTP newPreferences = allRegs(interval->registerType) & ~unpref; if (newPreferences != RBM_NONE) { +#ifdef DEBUG + if (VERBOSE) + { + printf("Last-use of V%02u between a PUTARG and CALL node. Removing following already-placed argument " + "registers " + "from its preferences: ", + compiler->lvaTrackedIndexToLclNum(varIndex)); + dumpRegMask(unpref); + printf("\n"); + } +#endif + interval->updateRegisterPreferences(newPreferences); } } @@ -3042,7 +3057,7 @@ RefPosition* LinearScan::BuildUse(GenTree* operand, regMaskTP candidates, int mu { unsigned varIndex = interval->getVarIndex(compiler); VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex); - PreferenceDyingLocal(interval); + UpdatePreferenceOfDyingLocal(interval); } #if FEATURE_PARTIAL_SIMD_CALLEE_SAVE buildUpperVectorRestoreRefPosition(interval, currentLoc, operand, true); @@ -3941,6 +3956,9 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) regMaskTP argMask = genRegMask(argReg); RefPosition* use = BuildUse(op1, argMask); + // Record that this register is occupied by a register now. + placedArgRegs |= argMask; + if (supportsSpecialPutArg() && isCandidateLocalRef(op1) && ((op1->gtFlags & GTF_VAR_DEATH) == 0)) { // This is the case for a "pass-through" copy of a lclVar. In the case where it is a non-last-use, @@ -3951,6 +3969,14 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) // Preference the destination to the interval of the first register defined by the first operand. assert(use->getInterval()->isLocalVar); isSpecialPutArg = true; + + // Record that this local is available in the register to ensure we + // keep the register in its local set if we see it die before the call + // (see UpdatePreferenceOfDyingLocal). + assert(numPlacedArgLocals < ArrLen(placedArgLocals)); + placedArgLocals[numPlacedArgLocals].VarIndex = use->getInterval()->getVarIndex(compiler); + placedArgLocals[numPlacedArgLocals].Reg = argReg; + numPlacedArgLocals++; } #ifdef TARGET_ARM @@ -3976,18 +4002,6 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) } } - placedArgRegs |= argMask; - - // If this is a passthrough tracked local then record it so that we can - // ensure we do not unpreference if we see a future use (see PreferenceDyingLocal). - if (isSpecialPutArg) - { - assert(numPlacedArgLocals < ArrLen(placedArgLocals)); - placedArgLocals[numPlacedArgLocals].VarIndex = use->getInterval()->getVarIndex(compiler); - placedArgLocals[numPlacedArgLocals].Reg = argReg; - numPlacedArgLocals++; - } - return srcCount; } From e867a02d9e2c26637ed00cda797796630d2c3e09 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 7 Oct 2022 23:08:53 +0200 Subject: [PATCH 7/8] Minor nits --- src/coreclr/jit/lsra.h | 2 +- src/coreclr/jit/lsrabuild.cpp | 18 ++++++++---------- 2 files changed, 9 insertions(+), 11 deletions(-) diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index 36bad01a6162d..9d9bac0a2770a 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -1842,7 +1842,7 @@ class LinearScan : public LinearScanInterface // These methods return the number of sources. int BuildNode(GenTree* tree); - void UpdatePreferenceOfDyingLocal(Interval* interval); + void UpdatePreferencesOfDyingLocal(Interval* interval); void getTgtPrefOperands(GenTree* tree, GenTree* op1, GenTree* op2, bool* prefOp1, bool* prefOp2); bool supportsSpecialPutArg(); diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 08157202fc2b6..ecbffedb64719 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -1735,7 +1735,7 @@ void LinearScan::buildRefPositionsForNode(GenTree* tree, LsraLocation currentLoc unsigned varIndex = varDsc->lvVarIndex; VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex); - UpdatePreferenceOfDyingLocal(getIntervalForLocalVar(varIndex)); + UpdatePreferencesOfDyingLocal(getIntervalForLocalVar(varIndex)); } } #else // TARGET_XARCH @@ -2952,7 +2952,7 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa } //------------------------------------------------------------------------ -// UpdatePreferenceOfDyingLocal: Update the preference of a dying local. +// UpdatePreferencesOfDyingLocal: Update the preference of a dying local. // // Arguments: // interval - the interval for the local @@ -2960,7 +2960,7 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa // Notes: // The "dying" information here is approximate, see the comment in BuildUse. // -void LinearScan::UpdatePreferenceOfDyingLocal(Interval* interval) +void LinearScan::UpdatePreferencesOfDyingLocal(Interval* interval) { assert(!VarSetOps::IsMember(compiler, currentLiveVars, interval->getVarIndex(compiler))); @@ -2999,21 +2999,19 @@ void LinearScan::UpdatePreferenceOfDyingLocal(Interval* interval) } } - regMaskTP newPreferences = allRegs(interval->registerType) & ~unpref; - if (newPreferences != RBM_NONE) + if (unpref != RBM_NONE) { #ifdef DEBUG if (VERBOSE) { - printf("Last-use of V%02u between a PUTARG and CALL node. Removing following already-placed argument " - "registers " - "from its preferences: ", + printf("Last use of V%02u between PUTARG and CALL. Removing occupied arg regs from preferences: ", compiler->lvaTrackedIndexToLclNum(varIndex)); dumpRegMask(unpref); printf("\n"); } #endif + regMaskTP newPreferences = allRegs(interval->registerType) & ~unpref; interval->updateRegisterPreferences(newPreferences); } } @@ -3057,7 +3055,7 @@ RefPosition* LinearScan::BuildUse(GenTree* operand, regMaskTP candidates, int mu { unsigned varIndex = interval->getVarIndex(compiler); VarSetOps::RemoveElemD(compiler, currentLiveVars, varIndex); - UpdatePreferenceOfDyingLocal(interval); + UpdatePreferencesOfDyingLocal(interval); } #if FEATURE_PARTIAL_SIMD_CALLEE_SAVE buildUpperVectorRestoreRefPosition(interval, currentLoc, operand, true); @@ -3972,7 +3970,7 @@ int LinearScan::BuildPutArgReg(GenTreeUnOp* node) // Record that this local is available in the register to ensure we // keep the register in its local set if we see it die before the call - // (see UpdatePreferenceOfDyingLocal). + // (see UpdatePreferencesOfDyingLocal). assert(numPlacedArgLocals < ArrLen(placedArgLocals)); placedArgLocals[numPlacedArgLocals].VarIndex = use->getInterval()->getVarIndex(compiler); placedArgLocals[numPlacedArgLocals].Reg = argReg; From 8c286c80fbab4d1badfd0c5148421566c8d544e9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 10 Oct 2022 11:28:08 +0200 Subject: [PATCH 8/8] Apply suggestions from code review Co-authored-by: Kunal Pathak --- src/coreclr/jit/lsrabuild.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index ecbffedb64719..7ddde7f82b997 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -2965,12 +2965,12 @@ void LinearScan::UpdatePreferencesOfDyingLocal(Interval* interval) assert(!VarSetOps::IsMember(compiler, currentLiveVars, interval->getVarIndex(compiler))); // If we see a use of a local between placing a register and a call then we - // want to update that local's preferences to exclude that register. - // Picking that register is otherwise going to force a spill. + // want to update that local's preferences to exclude the "placed" register. + // Picking the "placed" register is otherwise going to force a spill. // // We only need to do this on liveness updates because if the local is live // _after_ the call, then we are going to prefer callee-saved registers for - // it anyway, so there is no need to look at such local uses. + // such local anyway, so there is no need to look at such local uses. // if (placedArgRegs == RBM_NONE) {