From 17f8913019672ffcefd9a77aef2ca26cc8b7d2ba Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 5 Mar 2024 13:50:12 +0100 Subject: [PATCH 1/4] JIT: Validate some reg homing logic Just verifying that we never see float reg -> int reg homing on all our targets... --- src/coreclr/jit/codegencommon.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index bf2bfaa28aad47..5dfa73568ca96a 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -3795,6 +3795,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere regNumber begRegNum = genMapRegArgNumToRegNum(begReg, destMemType); GetEmitter()->emitIns_Mov(insCopy, size, xtraReg, begRegNum, /* canSkip */ false); + assert(!genIsValidIntReg(xtraReg) || !genIsValidFloatReg(begRegNum)); regSet.verifyRegUsed(xtraReg); @@ -3809,6 +3810,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere regNumber srcRegNum = genMapRegArgNumToRegNum(srcReg, destMemType); GetEmitter()->emitIns_Mov(insCopy, size, destRegNum, srcRegNum, /* canSkip */ false); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(srcRegNum)); regSet.verifyRegUsed(destRegNum); @@ -3860,6 +3862,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere regNumber destRegNum = genMapRegArgNumToRegNum(destReg, destMemType); GetEmitter()->emitIns_Mov(insCopy, size, destRegNum, xtraReg, /* canSkip */ false); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(xtraReg)); regSet.verifyRegUsed(destRegNum); /* mark the beginning register as processed */ @@ -3934,6 +3937,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere // todo -- suppress self move GetEmitter()->emitIns_R_R_I_I(INS_mov, EA_4BYTE, destRegNum, regNum, regArgTab[currentArgNum].slot - 1, 0); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(regNum)); regArgTab[currentArgNum].processed = true; regArgMaskLive &= ~genRegMask(regNum); } @@ -4107,6 +4111,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere } #endif inst_Mov(destMemType, destRegNum, regNum, /* canSkip */ false, size); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(regNum)); } /* mark the argument as processed */ @@ -4132,6 +4137,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere // Emit a shufpd with a 0 immediate, which preserves the 0th element of the dest reg // and moves the 0th element of the src reg into the 1st element of the dest reg. GetEmitter()->emitIns_R_R_I(INS_shufpd, emitActualTypeSize(varRegType), destRegNum, nextRegNum, 0); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(nextRegNum)); // Set destRegNum to regNum so that we skip the setting of the register below, // but mark argNum as processed and clear regNum from the live mask. destRegNum = regNum; @@ -4159,6 +4165,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere noway_assert(genIsValidFloatReg(nextRegNum)); noway_assert(genIsValidFloatReg(destRegNum)); GetEmitter()->emitIns_Mov(INS_mov, EA_8BYTE, destRegNum, nextRegNum, /* canSkip */ false); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(nextRegNum)); } } #if defined(TARGET_ARM64) && defined(FEATURE_SIMD) @@ -4179,6 +4186,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere noway_assert(genIsValidFloatReg(nextRegNum)); noway_assert(genIsValidFloatReg(destRegNum)); GetEmitter()->emitIns_R_R_I_I(INS_mov, EA_4BYTE, destRegNum, nextRegNum, i, 0); + assert(!genIsValidIntReg(destRegNum) || !genIsValidFloatReg(nextRegNum)); } } #endif // defined(TARGET_ARM64) && defined(FEATURE_SIMD) From 048f94c0b8b0ecbe55d8f9df37a121a86cea9962 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Mar 2024 13:27:45 +0100 Subject: [PATCH 2/4] Stop enregistering problematic case --- src/coreclr/jit/lclvars.cpp | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 55787580cc1df7..36c4773c56f238 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -3843,18 +3843,15 @@ var_types LclVarDsc::GetRegisterType(const GenTreeLclVarCommon* tree) const if (targetType == TYP_STRUCT) { - ClassLayout* layout; if (tree->OperIs(GT_LCL_FLD, GT_STORE_LCL_FLD)) { - layout = tree->AsLclFld()->GetLayout(); + targetType = tree->AsLclFld()->GetLayout()->GetRegisterType(); } else { assert((TypeGet() == TYP_STRUCT) && tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR)); - layout = GetLayout(); + targetType = GetRegisterType(); } - - targetType = layout->GetRegisterType(); } #ifdef DEBUG @@ -3888,7 +3885,12 @@ var_types LclVarDsc::GetRegisterType() const return TypeGet(); } assert(m_layout != nullptr); - return m_layout->GetRegisterType(); + var_types result = m_layout->GetRegisterType(); + if ((result != TYP_UNDEF) && varTypeUsesIntReg(result) && lvIsRegArg && !lvPromoted && genIsValidFloatReg(GetArgReg())) + { + result = TYP_UNDEF; + } + return result; } //------------------------------------------------------------------------ From 893f01f91cea0548310fb631ef3831d0401811ab Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Mar 2024 13:51:36 +0100 Subject: [PATCH 3/4] Better fix --- src/coreclr/jit/lclvars.cpp | 14 ++++++-------- src/coreclr/jit/lsra.cpp | 14 ++++++++++++++ 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/lclvars.cpp b/src/coreclr/jit/lclvars.cpp index 36c4773c56f238..55787580cc1df7 100644 --- a/src/coreclr/jit/lclvars.cpp +++ b/src/coreclr/jit/lclvars.cpp @@ -3843,15 +3843,18 @@ var_types LclVarDsc::GetRegisterType(const GenTreeLclVarCommon* tree) const if (targetType == TYP_STRUCT) { + ClassLayout* layout; if (tree->OperIs(GT_LCL_FLD, GT_STORE_LCL_FLD)) { - targetType = tree->AsLclFld()->GetLayout()->GetRegisterType(); + layout = tree->AsLclFld()->GetLayout(); } else { assert((TypeGet() == TYP_STRUCT) && tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR)); - targetType = GetRegisterType(); + layout = GetLayout(); } + + targetType = layout->GetRegisterType(); } #ifdef DEBUG @@ -3885,12 +3888,7 @@ var_types LclVarDsc::GetRegisterType() const return TypeGet(); } assert(m_layout != nullptr); - var_types result = m_layout->GetRegisterType(); - if ((result != TYP_UNDEF) && varTypeUsesIntReg(result) && lvIsRegArg && !lvPromoted && genIsValidFloatReg(GetArgReg())) - { - result = TYP_UNDEF; - } - return result; + return m_layout->GetRegisterType(); } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index e411de81ed80e3..35d32dfcb614e4 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -1681,6 +1681,20 @@ bool LinearScan::isRegCandidate(LclVarDsc* varDsc) return false; } + // Avoid allocating parameters that are passed in float regs into integer + // registers. We currently home float registers before integer registers, + // so that kind of enregistration can trash integer registers containing + // other parameters. + // We assume that these cases will be homed to float registers if they are + // promoted. + // TODO-CQ: Combine integer and float register homing to handle these kinds + // of conflicts. + if (varTypeUsesIntReg(varDsc->GetRegisterType()) && varDsc->lvIsRegArg && !varDsc->lvPromoted && genIsValidFloatReg(varDsc->GetArgReg())) + { + compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::IsStructArg)); + return false; + } + // Are we not optimizing and we have exception handlers? // if so mark all args and locals as volatile, so that they // won't ever get enregistered. From 33cdb8e95b90b29b7205e1963871b14878ed7a0b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 7 Mar 2024 13:52:40 +0100 Subject: [PATCH 4/4] Clean up --- src/coreclr/jit/lsra.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 35d32dfcb614e4..944b176af1018d 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -1689,7 +1689,8 @@ bool LinearScan::isRegCandidate(LclVarDsc* varDsc) // promoted. // TODO-CQ: Combine integer and float register homing to handle these kinds // of conflicts. - if (varTypeUsesIntReg(varDsc->GetRegisterType()) && varDsc->lvIsRegArg && !varDsc->lvPromoted && genIsValidFloatReg(varDsc->GetArgReg())) + if ((varDsc->TypeGet() == TYP_STRUCT) && varDsc->lvIsRegArg && !varDsc->lvPromoted && + varTypeUsesIntReg(varDsc->GetRegisterType()) && genIsValidFloatReg(varDsc->GetArgReg())) { compiler->lvaSetVarDoNotEnregister(lclNum DEBUGARG(DoNotEnregisterReason::IsStructArg)); return false;