diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index 8e7fb1f6b29f5e..8a9e8f06e4f428 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1144,6 +1144,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 ff1f2ccd7131ee..787b42846f47f7 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 13cdd22b21eb92..5386aa65a5d74b 100644 --- a/src/coreclr/jit/codegenlinear.cpp +++ b/src/coreclr/jit/codegenlinear.cpp @@ -1475,7 +1475,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; @@ -2338,6 +2338,30 @@ void CodeGen::genTransferRegGCState(regNumber dst, regNumber src) } #endif +//------------------------------------------------------------------------ +// genCodeForCatchArg: +// Generates code for GT_CATCH_ARG. +// +// Arguments: +// tree - the GT_CATCH_ARG 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 2b3cc0821a8791..5bca27b0247552 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -4375,14 +4375,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_LABEL: diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index fa450f08244ac8..86431cb877de10 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -4161,14 +4161,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_LABEL: diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 4afff4ae217058..2e80ef2477b0a5 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -747,7 +747,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); } } @@ -2104,14 +2103,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: @@ -4384,9 +4376,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 @@ -4410,6 +4399,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; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 67e4b5516684cc..3956943023ead6 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -6436,14 +6436,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/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/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 c749f11797941e..cf64f827cafa7e 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -1523,13 +1523,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 e6e9a6a67ee26b..6ba3fbd4ae84f4 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -211,164 +211,125 @@ 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) { assert(!interval->isLocalVar); 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; + regMaskTP inUse = regsBusyUntilKill | regsInUseThisLocation; + bool useRegConflict = (useRegAssignment & ~inUse.GetRegSetForType(interval->registerType)) == RBM_NONE; INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CONFLICT)); - if (!canChangeUseAssignment) + + if (defRefPosition->treeNode->IsMultiRegNode()) { - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_FIXED_DELAY_USE)); + // 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. } - if (defRefPosition->isFixedRegRef) + else { - defReg = defRefPosition->assignedReg(); - if (canChangeUseAssignment) + if (useRefPosition->isFixedRegRef) { -#ifdef DEBUG - RegRecord* defRegRecord = getRegisterRecord(defReg); - RefPosition* currFixedRegRefPosition = defRegRecord->recentRefPosition; - assert((currFixedRegRefPosition != nullptr) && - (currFixedRegRefPosition->nodeLocation == defRefPosition->nodeLocation)); -#endif + regNumber useReg = useRefPosition->assignedReg(); - 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(); + 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 (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) - { - // 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)) + if (!useRegConflict) { - useRegConflict = true; + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_DEF_IN_FIXED_USE, interval)); + defRefPosition->registerAssignment = useRegAssignment; + return; } } + else + { + useRegConflict = true; + } + } + if (defRefPosition->isFixedRegRef) + { if (!useRegConflict) { - // This is case #2. Use the useRegAssignment - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE2, 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 ((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)) - { - // 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; } - INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_CASE6, interval)); + INDEBUG(dumpLsraAllocationEvent(LSRA_EVENT_DEFUSE_COPY, interval)); return; }