From 18ef7f3b7715942614c4e0302f1cf8018854c84e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 2 May 2024 11:37:07 +0200 Subject: [PATCH 1/3] JIT: Mark swift error as busy before call definition RefPosition The RefPosition we were inserting here was inserted too late to actually protect the call definition from being allocated into the error register. Instead, we can just mark the existing `RefTypeFixedReg` created for the argument use as delay freed, which will have the intended effect of keeping the error register busy until after the call definition. --- src/coreclr/jit/lsraarmarch.cpp | 16 ++++++++++------ src/coreclr/jit/lsraxarch.cpp | 16 ++++++++++------ 2 files changed, 20 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/lsraarmarch.cpp b/src/coreclr/jit/lsraarmarch.cpp index cf5302dee9c71..8e30385ddfa2f 100644 --- a/src/coreclr/jit/lsraarmarch.cpp +++ b/src/coreclr/jit/lsraarmarch.cpp @@ -410,17 +410,21 @@ int LinearScan::BuildCall(GenTreeCall* call) // After a Swift call that might throw returns, we expect the error register to be consumed // by a GT_SWIFT_ERROR node. However, we want to ensure the error register won't be trashed // before GT_SWIFT_ERROR can consume it. - // (For example, the PInvoke epilog comes before the error register store.) + // (For example, by LSRA allocating the call's result to the same register.) // To do so, delay the freeing of the error register until the next node. // This only works if the next node after the call is the GT_SWIFT_ERROR node. - // (InsertPInvokeCallEpilog should have moved the GT_SWIFT_ERROR node during lowering.) + // (LowerNonvirtPinvokeCall should have moved the GT_SWIFT_ERROR node.) assert(call->gtNext != nullptr); assert(call->gtNext->OperIs(GT_SWIFT_ERROR)); - // We could use RefTypeKill, but RefTypeFixedReg is used less commonly, so the check for delayRegFree - // during register allocation should be cheaper in terms of TP. - RefPosition* pos = newRefPosition(REG_SWIFT_ERROR, currentLoc + 1, RefTypeFixedReg, call, RBM_SWIFT_ERROR); - setDelayFree(pos); + // Conveniently we model the zeroing of the register as a non-standard constant zero argument, + // which will have created a RefPosition corresponding to the use of the error at the location + // of the uses. Marking this RefPosition as delay freed has the effect of keeping the register + // busy at the location of the definition of the call. + RegRecord* swiftErrorRegRecord = getRegisterRecord(REG_SWIFT_ERROR); + assert((swiftErrorRegRecord != nullptr) && (swiftErrorRegRecord->lastRefPosition != nullptr) && + (swiftErrorRegRecord->lastRefPosition->nodeLocation == currentLoc)); + setDelayFree(swiftErrorRegRecord->lastRefPosition); } #endif // SWIFT_SUPPORT diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp index 7109c9087ac4e..bd4373d21d6b9 100644 --- a/src/coreclr/jit/lsraxarch.cpp +++ b/src/coreclr/jit/lsraxarch.cpp @@ -1386,17 +1386,21 @@ int LinearScan::BuildCall(GenTreeCall* call) // After a Swift call that might throw returns, we expect the error register to be consumed // by a GT_SWIFT_ERROR node. However, we want to ensure the error register won't be trashed // before GT_SWIFT_ERROR can consume it. - // (For example, the PInvoke epilog comes before the error register store.) + // (For example, by LSRA allocating the call's result to the same register.) // To do so, delay the freeing of the error register until the next node. // This only works if the next node after the call is the GT_SWIFT_ERROR node. - // (InsertPInvokeCallEpilog should have moved the GT_SWIFT_ERROR node during lowering.) + // (LowerNonvirtPinvokeCall should have moved the GT_SWIFT_ERROR node.) assert(call->gtNext != nullptr); assert(call->gtNext->OperIs(GT_SWIFT_ERROR)); - // We could use RefTypeKill, but RefTypeFixedReg is used less commonly, so the check for delayRegFree - // during register allocation should be cheaper in terms of TP. - RefPosition* pos = newRefPosition(REG_SWIFT_ERROR, currentLoc + 1, RefTypeFixedReg, call, RBM_SWIFT_ERROR); - setDelayFree(pos); + // Conveniently we model the zeroing of the register as a non-standard constant zero argument, + // which will have created a RefPosition corresponding to the use of the error at the location + // of the uses. Marking this RefPosition as delay freed has the effect of keeping the register + // busy at the location of the definition of the call. + RegRecord* swiftErrorRegRecord = getRegisterRecord(REG_SWIFT_ERROR); + assert((swiftErrorRegRecord != nullptr) && (swiftErrorRegRecord->lastRefPosition != nullptr) && + (swiftErrorRegRecord->lastRefPosition->nodeLocation == currentLoc)); + setDelayFree(swiftErrorRegRecord->lastRefPosition); } #endif // SWIFT_SUPPORT From 261a30e0c67e0ce3a5e8c06b26b281464b888d8d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 2 May 2024 15:44:16 +0200 Subject: [PATCH 2/3] Factor function --- src/coreclr/jit/lsra.h | 1 + src/coreclr/jit/lsraarmarch.cpp | 22 +--------------------- src/coreclr/jit/lsrabuild.cpp | 31 +++++++++++++++++++++++++++++++ src/coreclr/jit/lsraxarch.cpp | 22 +--------------------- 4 files changed, 34 insertions(+), 42 deletions(-) diff --git a/src/coreclr/jit/lsra.h b/src/coreclr/jit/lsra.h index fe09a2e456441..09f8b8d63e581 100644 --- a/src/coreclr/jit/lsra.h +++ b/src/coreclr/jit/lsra.h @@ -2062,6 +2062,7 @@ class LinearScan : public LinearScanInterface #endif int BuildPutArgReg(GenTreeUnOp* node); int BuildCall(GenTreeCall* call); + void MarkSwiftErrorBusyForCall(GenTreeCall* call); int BuildCmp(GenTree* tree); int BuildCmpOperands(GenTree* tree); int BuildBlockStore(GenTreeBlk* blkNode); diff --git a/src/coreclr/jit/lsraarmarch.cpp b/src/coreclr/jit/lsraarmarch.cpp index 8e30385ddfa2f..64bbe23b06a66 100644 --- a/src/coreclr/jit/lsraarmarch.cpp +++ b/src/coreclr/jit/lsraarmarch.cpp @@ -404,27 +404,7 @@ int LinearScan::BuildCall(GenTreeCall* call) #ifdef SWIFT_SUPPORT if (call->HasSwiftErrorHandling()) { - // Tree is a Swift call with error handling; error register should have been killed - assert((killMask & RBM_SWIFT_ERROR) != 0); - - // After a Swift call that might throw returns, we expect the error register to be consumed - // by a GT_SWIFT_ERROR node. However, we want to ensure the error register won't be trashed - // before GT_SWIFT_ERROR can consume it. - // (For example, by LSRA allocating the call's result to the same register.) - // To do so, delay the freeing of the error register until the next node. - // This only works if the next node after the call is the GT_SWIFT_ERROR node. - // (LowerNonvirtPinvokeCall should have moved the GT_SWIFT_ERROR node.) - assert(call->gtNext != nullptr); - assert(call->gtNext->OperIs(GT_SWIFT_ERROR)); - - // Conveniently we model the zeroing of the register as a non-standard constant zero argument, - // which will have created a RefPosition corresponding to the use of the error at the location - // of the uses. Marking this RefPosition as delay freed has the effect of keeping the register - // busy at the location of the definition of the call. - RegRecord* swiftErrorRegRecord = getRegisterRecord(REG_SWIFT_ERROR); - assert((swiftErrorRegRecord != nullptr) && (swiftErrorRegRecord->lastRefPosition != nullptr) && - (swiftErrorRegRecord->lastRefPosition->nodeLocation == currentLoc)); - setDelayFree(swiftErrorRegRecord->lastRefPosition); + MarkSwiftErrorBusyForCall(call); } #endif // SWIFT_SUPPORT diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 4a4af7725dd4d..5ffc8c087b85f 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -4479,3 +4479,34 @@ int LinearScan::BuildCmpOperands(GenTree* tree) srcCount += BuildOperandUses(op2, op2Candidates); return srcCount; } + +//------------------------------------------------------------------------ +// MarkSwiftErrorBusyForCall: Given a call set the appropriate RefTypeFixedReg +// RefPosition for the Swift error register as delay free to ensure the error +// register does not get allocated by LSRA before it has been consumed. +// +// Arguments: +// call - The call node +// +void LinearScan::MarkSwiftErrorBusyForCall(GenTreeCall* call) +{ + assert(call->HasSwiftErrorHandling()); + // After a Swift call that might throw returns, we expect the error register to be consumed + // by a GT_SWIFT_ERROR node. However, we want to ensure the error register won't be trashed + // before GT_SWIFT_ERROR can consume it. + // (For example, by LSRA allocating the call's result to the same register.) + // To do so, delay the freeing of the error register until the next node. + // This only works if the next node after the call is the GT_SWIFT_ERROR node. + // (LowerNonvirtPinvokeCall should have moved the GT_SWIFT_ERROR node.) + assert(call->gtNext != nullptr); + assert(call->gtNext->OperIs(GT_SWIFT_ERROR)); + + // Conveniently we model the zeroing of the register as a non-standard constant zero argument, + // which will have created a RefPosition corresponding to the use of the error at the location + // of the uses. Marking this RefPosition as delay freed has the effect of keeping the register + // busy at the location of the definition of the call. + RegRecord* swiftErrorRegRecord = getRegisterRecord(REG_SWIFT_ERROR); + assert((swiftErrorRegRecord != nullptr) && (swiftErrorRegRecord->lastRefPosition != nullptr) && + (swiftErrorRegRecord->lastRefPosition->nodeLocation == currentLoc)); + setDelayFree(swiftErrorRegRecord->lastRefPosition); +} diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp index bd4373d21d6b9..3d19214f8acbd 100644 --- a/src/coreclr/jit/lsraxarch.cpp +++ b/src/coreclr/jit/lsraxarch.cpp @@ -1380,27 +1380,7 @@ int LinearScan::BuildCall(GenTreeCall* call) #ifdef SWIFT_SUPPORT if (call->HasSwiftErrorHandling()) { - // Tree is a Swift call with error handling; error register should have been killed - assert((killMask & RBM_SWIFT_ERROR) != 0); - - // After a Swift call that might throw returns, we expect the error register to be consumed - // by a GT_SWIFT_ERROR node. However, we want to ensure the error register won't be trashed - // before GT_SWIFT_ERROR can consume it. - // (For example, by LSRA allocating the call's result to the same register.) - // To do so, delay the freeing of the error register until the next node. - // This only works if the next node after the call is the GT_SWIFT_ERROR node. - // (LowerNonvirtPinvokeCall should have moved the GT_SWIFT_ERROR node.) - assert(call->gtNext != nullptr); - assert(call->gtNext->OperIs(GT_SWIFT_ERROR)); - - // Conveniently we model the zeroing of the register as a non-standard constant zero argument, - // which will have created a RefPosition corresponding to the use of the error at the location - // of the uses. Marking this RefPosition as delay freed has the effect of keeping the register - // busy at the location of the definition of the call. - RegRecord* swiftErrorRegRecord = getRegisterRecord(REG_SWIFT_ERROR); - assert((swiftErrorRegRecord != nullptr) && (swiftErrorRegRecord->lastRefPosition != nullptr) && - (swiftErrorRegRecord->lastRefPosition->nodeLocation == currentLoc)); - setDelayFree(swiftErrorRegRecord->lastRefPosition); + MarkSwiftErrorBusyForCall(call); } #endif // SWIFT_SUPPORT From 4afb657231c5c7662ad5e3cf27524d7db408f355 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 2 May 2024 15:47:02 +0200 Subject: [PATCH 3/3] Fix build --- src/coreclr/jit/lsrabuild.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 5ffc8c087b85f..41b375acd4392 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -4480,6 +4480,7 @@ int LinearScan::BuildCmpOperands(GenTree* tree) return srcCount; } +#ifdef SWIFT_SUPPORT //------------------------------------------------------------------------ // MarkSwiftErrorBusyForCall: Given a call set the appropriate RefTypeFixedReg // RefPosition for the Swift error register as delay free to ensure the error @@ -4510,3 +4511,4 @@ void LinearScan::MarkSwiftErrorBusyForCall(GenTreeCall* call) (swiftErrorRegRecord->lastRefPosition->nodeLocation == currentLoc)); setDelayFree(swiftErrorRegRecord->lastRefPosition); } +#endif