From cb4faded9cc62beb89f947a5d36ff5c0b264c8cd Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Mar 2026 15:00:14 +0100 Subject: [PATCH 1/7] JIT: Avoid resolving def-use conflicts by changing use registers The only benefit is that it saves insertion of some explicit copies, but the trade off is an implicit contract between LSRA and codegen that is not very faithful. When LSRA defines in a different register than the constrained register, it believes that codegen can insert a copy and simultaneously use it from the new location, which is not true. --- src/coreclr/jit/lsra.cpp | 26 ++---- src/coreclr/jit/lsra.h | 11 +-- src/coreclr/jit/lsrabuild.cpp | 162 +++++++++++++--------------------- 3 files changed, 72 insertions(+), 127 deletions(-) diff --git a/src/coreclr/jit/lsra.cpp b/src/coreclr/jit/lsra.cpp index 734e0e67335d85..9baa4f25bdf108 100644 --- a/src/coreclr/jit/lsra.cpp +++ b/src/coreclr/jit/lsra.cpp @@ -11052,29 +11052,20 @@ void LinearScan::dumpLsraAllocationEvent(LsraDumpEvent event, printf("DUconflict "); dumpRegRecords(); break; - case LSRA_EVENT_DEFUSE_CASE1: - printf(indentFormat, " Case #1 use defRegAssignment"); + case LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE: + printf(indentFormat, " Define in fixed use reg"); dumpRegRecords(); break; - case LSRA_EVENT_DEFUSE_CASE2: - printf(indentFormat, " Case #2 use useRegAssignment"); + case LSRA_EVENT_DEFUSE_DEF_IN_USE: + printf(indentFormat, " Define in candidate use reg"); dumpRegRecords(); break; - case LSRA_EVENT_DEFUSE_CASE3: - printf(indentFormat, " Case #3 use useRegAssignment"); - dumpRegRecords(); - dumpRegRecords(); - break; - case LSRA_EVENT_DEFUSE_CASE4: - printf(indentFormat, " Case #4 use defRegAssignment"); - dumpRegRecords(); - break; - case LSRA_EVENT_DEFUSE_CASE5: - printf(indentFormat, " Case #5 set def to all regs"); + case LSRA_EVENT_DEFUSE_ANY_DEF: + printf(indentFormat, " Define in any reg"); dumpRegRecords(); break; - case LSRA_EVENT_DEFUSE_CASE6: - printf(indentFormat, " Case #6 need a copy"); + case LSRA_EVENT_DEFUSE_COPY: + printf(indentFormat, " Need a copy"); dumpRegRecords(); if (interval == nullptr) { @@ -11247,7 +11238,6 @@ void LinearScan::dumpLsraAllocationEvent(LsraDumpEvent event, break; // We currently don't dump anything for these events. - case LSRA_EVENT_DEFUSE_FIXED_DELAY_USE: case LSRA_EVENT_SPILL_EXTENDED_LIFETIME: case LSRA_EVENT_END_BB: case LSRA_EVENT_FREE_REGS: diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index 9d60406683668c..c7316e6ce6e224 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -1522,13 +1522,10 @@ class LinearScan : public RegAllocInterface { // Conflicting def/use LSRA_EVENT_DEFUSE_CONFLICT, - LSRA_EVENT_DEFUSE_FIXED_DELAY_USE, - LSRA_EVENT_DEFUSE_CASE1, - LSRA_EVENT_DEFUSE_CASE2, - LSRA_EVENT_DEFUSE_CASE3, - LSRA_EVENT_DEFUSE_CASE4, - LSRA_EVENT_DEFUSE_CASE5, - LSRA_EVENT_DEFUSE_CASE6, + LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE, + LSRA_EVENT_DEFUSE_DEF_IN_USE, + LSRA_EVENT_DEFUSE_ANY_DEF, + LSRA_EVENT_DEFUSE_COPY, // Spilling LSRA_EVENT_SPILL, diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 79ddcc5b1c732e..5e00b2241f4f70 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -211,40 +211,44 @@ RefPosition* LinearScan::newRefPositionRaw(LsraLocation nodeLocation, GenTree* t // The two RefPositions are for the same interval, which is a tree-temp. // // Notes: -// We require some special handling for the case where the use is a "delayRegFree" case of a fixedReg. -// In that case, if we change the registerAssignment on the useRefPosition, we will lose the fact that, -// even if we assign a different register (and rely on codegen to do the copy), that fixedReg also needs -// to remain busy until the Def register has been allocated. In that case, we don't allow Case 1 or Case 4 -// below. -// Here are the cases we consider (in this order): -// 1. If The defRefPosition specifies a single register, and there are no conflicting -// FixedReg uses of it between the def and use, we use that register, and the code generator -// will insert the copy. Note that it cannot be in use because there is a FixedRegRef for the def. -// 2. If the useRefPosition specifies a single register, and it is not in use, and there are no -// conflicting FixedReg uses of it between the def and use, we use that register, and the code generator -// will insert the copy. -// 3. If the defRefPosition specifies a single register (but there are conflicts, as determined -// in 1.), and there are no conflicts with the useRefPosition register (if it's a single register), -/// we set the register requirements on the defRefPosition to the use registers, and the -// code generator will insert a copy on the def. We can't rely on the code generator to put a copy -// on the use if it has multiple possible candidates, as it won't know which one has been allocated. -// 4. If the useRefPosition specifies a single register, and there are no conflicts with the register -// on the defRefPosition, we leave the register requirements on the defRefPosition as-is, and set -// the useRefPosition to the def registers, for similar reasons to case #3. -// 5. If both the defRefPosition and the useRefPosition specify single registers, but both have conflicts, -// We set the candidates on defRefPosition to be all regs of the appropriate type, and since they are -// single registers, codegen can insert the copy. -// 6. Finally, if the RefPositions specify disjoint subsets of the registers (or the use is fixed but -// has a conflict), we must insert a copy. The copy will be inserted before the use if the -// use is not fixed (in the fixed case, the code generator will insert the use). -// -// TODO-CQ: We get bad register allocation in case #3 in the situation where no register is -// available for the lifetime. We end up allocating a register that must be spilled, and it probably -// won't be the register that is actually defined by the target instruction. So, we have to copy it -// and THEN spill it. In this case, we should be using the def requirement. But we need to change -// the interface to this method a bit to make that work (e.g. returning a candidate set to use, but -// leaving the registerAssignment as-is on the def, so that if we find that we need to spill anyway -// we can use the fixed-reg on the def. +// In general we only ever change registers on the def. For uses we instead +// prefer to insert an explicit copy in LSRA. That guarantees that the +// actual use register remains modelled by LSRA on the use. +// +// The cases we consider otherwise are: +// +// - (Inherit fixed use reg) If the useRefPosition specifies a single +// register, and it is not in use, and there are no conflicting FixedReg +// uses of it between the def and use, we use that register, and the code +// generator will insert the copy. +// +// - (Inherit all use regs) If the defRefPosition specifies a single +// register, and there are no conflicts with the useRefPosition register (if +// it's a single register), we set the register requirements on the +// defRefPosition to the use registers, and the code generator will insert a +// copy on the def. We can't rely on the code generator to put a copy on +// the use if it has multiple possible candidates, as it won't know which +// one has been allocated. +// +// - (Any def reg) If both the defRefPosition and the useRefPosition specify +// single registers, but we couldn't update the def reg, we set the +// candidates on defRefPosition to be all regs of the appropriate type, and +// since they are single registers, codegen can insert the copy. +// +// - (Insert copy) Finally, if the RefPositions specify disjoint subsets of +// the registers (or the use is fixed but has a conflict), we must insert a +// copy. The copy will be inserted before the use if the use is not fixed +// (in the fixed case, the code generator will insert the use). +// +// TODO-CQ: We get bad register allocation in the (Inherit all use regs) case +// in the situation where no register is available for the lifetime. We end up +// allocating a register that must be spilled, and it probably won't be the +// register that is actually defined by the target instruction. So, we have to +// copy it and THEN spill it. In this case, we should be using the def +// requirement. But we need to change the interface to this method a bit to +// make that work (e.g. returning a candidate set to use, but leaving the +// registerAssignment as-is on the def, so that if we find that we need to +// spill anyway we can use the fixed-reg on the def. // void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* defRefPosition) { @@ -253,50 +257,12 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de RefPosition* useRefPosition = defRefPosition->nextRefPosition; SingleTypeRegSet defRegAssignment = defRefPosition->registerAssignment; SingleTypeRegSet useRegAssignment = useRefPosition->registerAssignment; - regNumber defReg = REG_NA; - regNumber useReg = REG_NA; - bool defRegConflict = false; bool useRegConflict = false; - // If the useRefPosition is a "delayRegFree", we can't change the registerAssignment - // on it, or we will fail to ensure that the fixedReg is busy at the time the target - // (of the node that uses this interval) is allocated. - bool canChangeUseAssignment = !useRefPosition->isFixedRegRef || !useRefPosition->delayRegFree; - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CONFLICT)); - if (!canChangeUseAssignment) - { - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_FIXED_DELAY_USE)); - } - if (defRefPosition->isFixedRegRef) - { - defReg = defRefPosition->assignedReg(); - if (canChangeUseAssignment) - { -#ifdef DEBUG - RegRecord* defRegRecord = getRegisterRecord(defReg); - RefPosition* currFixedRegRefPosition = defRegRecord->recentRefPosition; - assert((currFixedRegRefPosition != nullptr) && - (currFixedRegRefPosition->nodeLocation == defRefPosition->nodeLocation)); -#endif - - LsraLocation nextRegLoc = getNextFixedRef(defReg, defRefPosition->getRegisterType()); - if (nextRegLoc > useRefPosition->getRefEndLocation()) - { - // This is case #1. Use the defRegAssignment - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE1)); - useRefPosition->registerAssignment = defRegAssignment; - return; - } - else - { - defRegConflict = true; - } - } - } if (useRefPosition->isFixedRegRef) { - useReg = useRefPosition->assignedReg(); + regNumber useReg = useRefPosition->assignedReg(); LsraLocation nextRegLoc = getNextFixedRef(useReg, useRefPosition->getRegisterType()); @@ -329,8 +295,7 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de } if (!useRegConflict) { - // This is case #2. Use the useRegAssignment - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE2, interval)); + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE, interval)); defRefPosition->registerAssignment = useRegAssignment; return; } @@ -340,35 +305,28 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de useRegConflict = true; } } - if ((defReg != REG_NA) && !useRegConflict) - { - // This is case #3. - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE3, interval)); - defRefPosition->registerAssignment = useRegAssignment; - defRefPosition->isFixedRegRef = false; - return; - } - if ((useReg != REG_NA) && !defRegConflict && canChangeUseAssignment) - { - // This is case #4. - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE4, interval)); - useRefPosition->registerAssignment = defRegAssignment; - useRefPosition->isFixedRegRef = false; - return; - } - if ((defReg != REG_NA) && (useReg != REG_NA)) + if (defRefPosition->isFixedRegRef) { - // This is case #5. - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE5, interval)); - RegisterType regType = interval->registerType; - assert((getRegisterType(interval, defRefPosition) == regType) && - (getRegisterType(interval, useRefPosition) == regType)); - SingleTypeRegSet candidates = allRegs(regType); - defRefPosition->registerAssignment = candidates; - defRefPosition->isFixedRegRef = false; - return; + if (!useRegConflict) + { + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_USE, interval)); + defRefPosition->registerAssignment = useRegAssignment; + defRefPosition->isFixedRegRef = false; + return; + } + if (useRefPosition->isFixedRegRef) + { + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_ANY_DEF, interval)); + RegisterType regType = interval->registerType; + assert((getRegisterType(interval, defRefPosition) == regType) && + (getRegisterType(interval, useRefPosition) == regType)); + SingleTypeRegSet candidates = allRegs(regType); + defRefPosition->registerAssignment = candidates; + defRefPosition->isFixedRegRef = false; + return; + } } - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE6, interval)); + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_COPY, interval)); return; } From d3795f91b6dbf2a127c25a14d26038ddc795ad23 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 9 Mar 2026 15:17:30 +0100 Subject: [PATCH 2/7] Feedback --- src/coreclr/jit/lsrabuild.cpp | 1 - 1 file changed, 1 deletion(-) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 5e00b2241f4f70..aeaae52848b26c 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -255,7 +255,6 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de assert(!interval->isLocalVar); RefPosition* useRefPosition = defRefPosition->nextRefPosition; - SingleTypeRegSet defRegAssignment = defRefPosition->registerAssignment; SingleTypeRegSet useRegAssignment = useRefPosition->registerAssignment; bool useRegConflict = false; From 891feb0886b82c4f67be4dad01a1959f9faf9d9a Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 10 Mar 2026 13:45:52 +0100 Subject: [PATCH 3/7] Fix GT_CATCH_ARG codegen --- src/coreclr/jit/codegen.h | 1 + src/coreclr/jit/codegenarmarch.cpp | 9 +-------- src/coreclr/jit/codegenlinear.cpp | 26 +++++++++++++++++++++++++- src/coreclr/jit/codegenloongarch64.cpp | 9 +-------- src/coreclr/jit/codegenriscv64.cpp | 9 +-------- src/coreclr/jit/codegenxarch.cpp | 9 +-------- 6 files changed, 30 insertions(+), 33 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 8472dfe8c0233d..2e9d4ad87745a6 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1136,6 +1136,7 @@ class CodeGen final : public CodeGenInterface void genCodeForShiftRMW(GenTreeStoreInd* storeInd); #endif // TARGET_XARCH + void genCodeForCatchArg(GenTree* tree); void genCodeForCast(GenTreeOp* tree); void genCodeForLclAddr(GenTreeLclFld* lclAddrNode); void genCodeForIndexAddr(GenTreeIndexAddr* tree); diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index fcef600921e254..a8cf73f95bc6ec 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -500,14 +500,7 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode) break; case GT_CATCH_ARG: - - noway_assert(handlerGetsXcptnObj(m_compiler->compCurBB->GetCatchType())); - - /* Catch arguments get passed in a register. genCodeForBBlist() - would have marked it as holding a GC object, but not used. */ - - noway_assert(gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT); - genConsumeReg(treeNode); + genCodeForCatchArg(treeNode); break; case GT_ASYNC_CONTINUATION: diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 7823547e9edbe2..82d3207af5aeca 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -1461,7 +1461,7 @@ void CodeGen::genCheckConsumeNode(GenTree* const node) } } - assert(node->OperIs(GT_CATCH_ARG) || ((node->gtDebugFlags & GTF_DEBUG_NODE_CG_CONSUMED) == 0)); + assert((node->gtDebugFlags & GTF_DEBUG_NODE_CG_CONSUMED) == 0); assert((lastConsumedNode == nullptr) || (node->gtUseNum == -1) || (node->gtUseNum > lastConsumedNode->gtUseNum)); node->gtDebugFlags |= GTF_DEBUG_NODE_CG_CONSUMED; @@ -2324,6 +2324,30 @@ void CodeGen::genTransferRegGCState(regNumber dst, regNumber src) } #endif +//------------------------------------------------------------------------ +// genCodeForCatchArg: +// Generates code for GT_CATCH_ARG. +// +// Arguments: +// tree - the GT_CAST node. +// +void CodeGen::genCodeForCatchArg(GenTree* tree) +{ + noway_assert(handlerGetsXcptnObj(m_compiler->compCurBB->GetCatchType())); + + // Catch arguments get passed in a register. genCodeForBBlist() + // would have marked it as holding a GC object, but not used. + + noway_assert(gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT); + inst_Mov(TYP_REF, tree->GetRegNum(), REG_EXCEPTION_OBJECT, /* canSkip */ true); + + if (tree->GetRegNum() != REG_EXCEPTION_OBJECT) + { + gcInfo.gcMarkRegSetNpt(RBM_EXCEPTION_OBJECT); + } + genProduceReg(tree); +} + //------------------------------------------------------------------------ // genCodeForCast: Generates the code for GT_CAST. // diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index feb4d28396a082..295806ff09128f 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -4369,14 +4369,7 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode) break; case GT_CATCH_ARG: - - noway_assert(handlerGetsXcptnObj(m_compiler->compCurBB->GetCatchType())); - - /* Catch arguments get passed in a register. genCodeForBBlist() - would have marked it as holding a GC object, but not used. */ - - noway_assert(gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT); - genConsumeReg(treeNode); + genCodeForCatchArg(treeNode); break; case GT_PINVOKE_PROLOG: diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index 148a42c53c98b1..2d2ae13051a7fa 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -4168,14 +4168,7 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode) break; case GT_CATCH_ARG: - - noway_assert(handlerGetsXcptnObj(m_compiler->compCurBB->GetCatchType())); - - /* Catch arguments get passed in a register. genCodeForBBlist() - would have marked it as holding a GC object, but not used. */ - - noway_assert(gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT); - genConsumeReg(treeNode); + genCodeForCatchArg(treeNode); break; case GT_PINVOKE_PROLOG: diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 36a120250c6cf3..195965caad98bc 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -2109,14 +2109,7 @@ void CodeGen::genCodeForTreeNode(GenTree* treeNode) break; case GT_CATCH_ARG: - - noway_assert(handlerGetsXcptnObj(m_compiler->compCurBB->GetCatchType())); - - /* Catch arguments get passed in a register. genCodeForBBlist() - would have marked it as holding a GC object, but not used. */ - - noway_assert(gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT); - genConsumeReg(treeNode); + genCodeForCatchArg(treeNode); break; case GT_ASYNC_CONTINUATION: From 6c8262518d554b033b49f51bdb0f7ac7dcf4af0f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 10 Mar 2026 14:03:00 +0100 Subject: [PATCH 4/7] Codegen fixes --- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/codegenxarch.cpp | 6 ++---- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 82d3207af5aeca..7672da283a7657 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -2336,7 +2336,7 @@ void CodeGen::genCodeForCatchArg(GenTree* tree) noway_assert(handlerGetsXcptnObj(m_compiler->compCurBB->GetCatchType())); // Catch arguments get passed in a register. genCodeForBBlist() - // would have marked it as holding a GC object, but not used. + // would have marked it as holding a GC object, but not used. noway_assert(gcInfo.gcRegGCrefSetCur & RBM_EXCEPTION_OBJECT); inst_Mov(TYP_REF, tree->GetRegNum(), REG_EXCEPTION_OBJECT, /* canSkip */ true); diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 195965caad98bc..9334a7cc7463a1 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -752,7 +752,6 @@ void CodeGen::genCodeForMulHi(GenTreeOp* treeNode) // Move the result to the desired register, if necessary if (treeNode->OperIs(GT_MULHI)) { - assert(targetReg == REG_RDX); inst_Mov(targetType, targetReg, REG_RDX, /* canSkip */ true); } } @@ -4392,9 +4391,6 @@ void CodeGen::genLockedInstructions(GenTreeOp* node) // When value is used (it's the original value of the memory location) // we fallback to cmpxchg-loop idiom. - // for cmpxchg we need to keep the original value in RAX - assert(node->GetRegNum() == REG_RAX); - // mov RAX, dword ptr [addrReg] //.LOOP: // mov tmp, RAX @@ -4418,6 +4414,8 @@ void CodeGen::genLockedInstructions(GenTreeOp* node) inst_JMP(EJ_jne, loop); gcInfo.gcMarkRegSetNpt(genRegMask(addr->GetRegNum())); + inst_Mov(node->TypeGet(), node->GetRegNum(), REG_RAX, /* canSkip */ true); + genProduceReg(node); } return; From ffdccca2c02f6f848873d1047ee19c3c1d74c3ea Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 1 Apr 2026 13:01:45 +0200 Subject: [PATCH 5/7] Avoid changing use regs to all-in-use registers --- src/coreclr/jit/codegenlinear.cpp | 2 +- src/coreclr/jit/lsrabuild.cpp | 14 +++----------- 2 files changed, 4 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/codegenlinear.cpp b/src/coreclr/jit/codegenlinear.cpp index 179e7940ecfee8..0f32cc666260f2 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -2339,7 +2339,7 @@ void CodeGen::genTransferRegGCState(regNumber dst, regNumber src) // Generates code for GT_CATCH_ARG. // // Arguments: -// tree - the GT_CAST node. +// tree - the GT_CATCH_ARG node. // void CodeGen::genCodeForCatchArg(GenTree* tree) { diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 9a4761886ea0c7..56344d55cac4bf 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -256,7 +256,8 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de RefPosition* useRefPosition = defRefPosition->nextRefPosition; SingleTypeRegSet useRegAssignment = useRefPosition->registerAssignment; - bool useRegConflict = false; + regMaskTP inUse = regsBusyUntilKill | regsInUseThisLocation; + bool useRegConflict = (useRegAssignment & ~inUse.GetRegSetForType(interval->registerType)) == RBM_NONE; INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CONFLICT)); if (useRefPosition->isFixedRegRef) @@ -274,7 +275,7 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de // OK, no conflicting FixedReg references. // Now, check to see whether it is currently in use. RegRecord* useRegRecord = getRegisterRecord(useReg); - if (useRegRecord->assignedInterval != nullptr) + if (!useRegConflict && (useRegRecord->assignedInterval != nullptr)) { RefPosition* possiblyConflictingRef = useRegRecord->assignedInterval->recentRefPosition; LsraLocation possiblyConflictingRefLocation = possiblyConflictingRef->getRefEndLocation(); @@ -284,15 +285,6 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de } } if (!useRegConflict) - { - // The use-reg may be busy at this point due to being a - // delay-free use from the previous location. - if (isRegInUse(useReg, interval->registerType)) - { - useRegConflict = true; - } - } - if (!useRegConflict) { INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE, interval)); defRefPosition->registerAssignment = useRegAssignment; From c7a1c5f499d050f347c31c36d97cfed2d5533521 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Apr 2026 16:05:39 +0200 Subject: [PATCH 6/7] Do not change multireg defs --- src/coreclr/jit/hwintrinsiccodegenxarch.cpp | 3 +++ src/coreclr/jit/lsrabuild.cpp | 10 ++++++++++ 2 files changed, 13 insertions(+) diff --git a/src/coreclr/jit/hwintrinsiccodegenxarch.cpp b/src/coreclr/jit/hwintrinsiccodegenxarch.cpp index 0c115532c755ea..9487df441fc5ff 100644 --- a/src/coreclr/jit/hwintrinsiccodegenxarch.cpp +++ b/src/coreclr/jit/hwintrinsiccodegenxarch.cpp @@ -2546,6 +2546,9 @@ void CodeGen::genX86BaseIntrinsic(GenTreeHWIntrinsic* node, insOpts instOptions) // emit the DIV/IDIV instruction emit->emitInsBinary(ins, attr, node, op3); + assert(node->GetRegNumByIdx(0) == REG_EAX); + assert(node->GetRegNumByIdx(1) == REG_EDX); + break; } diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 56344d55cac4bf..8ccb861d1a59ee 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -254,6 +254,16 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de { assert(!interval->isLocalVar); + if (defRefPosition->treeNode->IsMultiRegNode()) + { + // If the defRefPosition is multireg then we cannot change any of its + // register assignments. That could cause us to require a parallel + // assignment during codegen that could have cycles in it. For example, + // x64 DivRem always defines into RAX and RDX, so it would be a problem + // to change the register assignments to RDX and RAX respectively. + return; + } + RefPosition* useRefPosition = defRefPosition->nextRefPosition; SingleTypeRegSet useRegAssignment = useRefPosition->registerAssignment; regMaskTP inUse = regsBusyUntilKill | regsInUseThisLocation; From 27f63381bfa633907df849ec72c9b762ff00ccb5 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Apr 2026 16:16:35 +0200 Subject: [PATCH 7/7] Fixes --- src/coreclr/jit/gentree.h | 6 +- src/coreclr/jit/lsrabuild.cpp | 102 +++++++++++++++++----------------- 2 files changed, 55 insertions(+), 53 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 178cfd3c48e864..6efef0a77bf6fe 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -6443,14 +6443,14 @@ struct GenTreeJitIntrinsic : public GenTreeMultiOp // regNumber GetRegNumByIdx(unsigned idx) const { -#ifdef TARGET_ARM64 - assert(idx < MAX_MULTIREG_COUNT); - if (idx == 0) { return GetRegNum(); } +#ifdef TARGET_ARM64 + assert(idx < MAX_MULTIREG_COUNT); + if (NeedsConsecutiveRegisters()) { assert(IsMultiRegNode()); diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 8ccb861d1a59ee..4ce7b153a74e8f 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -254,6 +254,13 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de { assert(!interval->isLocalVar); + RefPosition* useRefPosition = defRefPosition->nextRefPosition; + SingleTypeRegSet useRegAssignment = useRefPosition->registerAssignment; + regMaskTP inUse = regsBusyUntilKill | regsInUseThisLocation; + bool useRegConflict = (useRegAssignment & ~inUse.GetRegSetForType(interval->registerType)) == RBM_NONE; + + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CONFLICT)); + if (defRefPosition->treeNode->IsMultiRegNode()) { // If the defRefPosition is multireg then we cannot change any of its @@ -261,70 +268,65 @@ void LinearScan::resolveConflictingDefAndUse(Interval* interval, RefPosition* de // assignment during codegen that could have cycles in it. For example, // x64 DivRem always defines into RAX and RDX, so it would be a problem // to change the register assignments to RDX and RAX respectively. - return; } - - RefPosition* useRefPosition = defRefPosition->nextRefPosition; - SingleTypeRegSet useRegAssignment = useRefPosition->registerAssignment; - regMaskTP inUse = regsBusyUntilKill | regsInUseThisLocation; - bool useRegConflict = (useRegAssignment & ~inUse.GetRegSetForType(interval->registerType)) == RBM_NONE; - - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CONFLICT)); - if (useRefPosition->isFixedRegRef) + else { - regNumber useReg = useRefPosition->assignedReg(); + if (useRefPosition->isFixedRegRef) + { + regNumber useReg = useRefPosition->assignedReg(); - LsraLocation nextRegLoc = getNextFixedRef(useReg, useRefPosition->getRegisterType()); + LsraLocation nextRegLoc = getNextFixedRef(useReg, useRefPosition->getRegisterType()); - // We know that useRefPosition is a fixed use, so there is a next reference. - assert(nextRegLoc <= useRefPosition->nodeLocation); + // We know that useRefPosition is a fixed use, so there is a next reference. + assert(nextRegLoc <= useRefPosition->nodeLocation); - // First, check to see if there are any conflicting FixedReg references between the def and use. - if (nextRegLoc == useRefPosition->nodeLocation) - { - // OK, no conflicting FixedReg references. - // Now, check to see whether it is currently in use. - RegRecord* useRegRecord = getRegisterRecord(useReg); - if (!useRegConflict && (useRegRecord->assignedInterval != nullptr)) + // First, check to see if there are any conflicting FixedReg references between the def and use. + if (nextRegLoc == useRefPosition->nodeLocation) { - RefPosition* possiblyConflictingRef = useRegRecord->assignedInterval->recentRefPosition; - LsraLocation possiblyConflictingRefLocation = possiblyConflictingRef->getRefEndLocation(); - if (possiblyConflictingRefLocation >= defRefPosition->nodeLocation) + // OK, no conflicting FixedReg references. + // Now, check to see whether it is currently in use. + RegRecord* useRegRecord = getRegisterRecord(useReg); + if (!useRegConflict && (useRegRecord->assignedInterval != nullptr)) { - useRegConflict = true; + RefPosition* possiblyConflictingRef = useRegRecord->assignedInterval->recentRefPosition; + LsraLocation possiblyConflictingRefLocation = possiblyConflictingRef->getRefEndLocation(); + if (possiblyConflictingRefLocation >= defRefPosition->nodeLocation) + { + useRegConflict = true; + } } + if (!useRegConflict) + { + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE, interval)); + defRefPosition->registerAssignment = useRegAssignment; + return; + } + } + else + { + useRegConflict = true; } + } + if (defRefPosition->isFixedRegRef) + { if (!useRegConflict) { - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE, interval)); + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_USE, interval)); defRefPosition->registerAssignment = useRegAssignment; + defRefPosition->isFixedRegRef = false; + return; + } + if (useRefPosition->isFixedRegRef) + { + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_ANY_DEF, interval)); + RegisterType regType = interval->registerType; + assert((getRegisterType(interval, defRefPosition) == regType) && + (getRegisterType(interval, useRefPosition) == regType)); + SingleTypeRegSet candidates = allRegs(regType); + defRefPosition->registerAssignment = candidates; + defRefPosition->isFixedRegRef = false; return; } - } - else - { - useRegConflict = true; - } - } - if (defRefPosition->isFixedRegRef) - { - if (!useRegConflict) - { - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_USE, interval)); - defRefPosition->registerAssignment = useRegAssignment; - defRefPosition->isFixedRegRef = false; - return; - } - if (useRefPosition->isFixedRegRef) - { - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_ANY_DEF, interval)); - RegisterType regType = interval->registerType; - assert((getRegisterType(interval, defRefPosition) == regType) && - (getRegisterType(interval, useRefPosition) == regType)); - SingleTypeRegSet candidates = allRegs(regType); - defRefPosition->registerAssignment = candidates; - defRefPosition->isFixedRegRef = false; - return; } } INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_COPY, interval));