From 20205d8fec7cf5ea642dc15c1db40b6c51c85224 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 23 Nov 2022 13:54:56 +0100 Subject: [PATCH 1/2] JIT: Ignore source indir when checking interference for block store source address MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit For block stores we can contain both the source and destination address. Since the source is a value we will have an indirection on top of the address, such as N017 ( 6, 14) [000001] #---G------ t1 = ▌ IND ref REG x1 $240 ┌──▌ t1 ref N019 ( 8, 17) [000003] -c--G------ t3 = ▌ LEA(b+8) byref REG NA ┌──▌ t3 byref N021 ( 32, 39) [000004] nc-XG--N--- t4 = ▌ IND struct REG NA N023 (???,???) [000025] Dc--------- t25 = LCL_FLD_ADDR byref V03 tmp1 [+0] NA REG NA ┌──▌ t25 byref ├──▌ t4 struct N025 ( 42, 46) [000017] sA--------- ▌ STORE_BLK struct (copy) (Unroll) REG NA where [000004] is the source indirection and [000003] is the source address. The existing containment check was checking interference of [000003] with [000004], which is conservative given that the indirection itself is always contained. --- src/coreclr/jit/lower.h | 2 +- src/coreclr/jit/lowerarmarch.cpp | 11 ++++++----- src/coreclr/jit/lowerloongarch64.cpp | 12 +++++++----- src/coreclr/jit/lowerxarch.cpp | 11 ++++++----- 4 files changed, 20 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 63174ccec9ff11..4e2de370165fe4 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -317,7 +317,7 @@ class Lowering final : public Phase GenTree* LowerSignedDivOrMod(GenTree* node); void LowerBlockStore(GenTreeBlk* blkNode); void LowerBlockStoreCommon(GenTreeBlk* blkNode); - void ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr); + void ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent); void LowerPutArgStkOrSplit(GenTreePutArgStk* putArgNode); #ifdef TARGET_XARCH void LowerPutArgStk(GenTreePutArgStk* putArgStk); diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 0ac3ea9d47c286..5901cba7a2870c 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -566,7 +566,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) src->AsIntCon()->SetIconValue(fill); - ContainBlockStoreAddress(blkNode, size, dstAddr); + ContainBlockStoreAddress(blkNode, size, dstAddr, nullptr); } else { @@ -637,10 +637,10 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) if (src->OperIs(GT_IND)) { - ContainBlockStoreAddress(blkNode, size, src->AsIndir()->Addr()); + ContainBlockStoreAddress(blkNode, size, src->AsIndir()->Addr(), src->AsIndir()); } - ContainBlockStoreAddress(blkNode, size, dstAddr); + ContainBlockStoreAddress(blkNode, size, dstAddr, nullptr); } else { @@ -658,8 +658,9 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) // blkNode - the block store node // size - the block size // addr - the address node to try to contain +// addrParent - the parent of addr, in case this is checking containment the source address. // -void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr) +void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent) { assert(blkNode->OperIs(GT_STORE_BLK) && (blkNode->gtBlkOpKind == GenTreeBlk::BlkOpKindUnroll)); assert(size < INT32_MAX); @@ -692,7 +693,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT } #endif // !TARGET_ARM - if (!IsSafeToContainMem(blkNode, addr)) + if (!IsSafeToContainMem(blkNode, addrParent, addr)) { return; } diff --git a/src/coreclr/jit/lowerloongarch64.cpp b/src/coreclr/jit/lowerloongarch64.cpp index f2dbb3ce42cc29..68002d1771e03a 100644 --- a/src/coreclr/jit/lowerloongarch64.cpp +++ b/src/coreclr/jit/lowerloongarch64.cpp @@ -252,7 +252,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) } src->AsIntCon()->SetIconValue(fill); - ContainBlockStoreAddress(blkNode, size, dstAddr); + ContainBlockStoreAddress(blkNode, size, dstAddr, nullptr); } else { @@ -307,10 +307,10 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) if (src->OperIs(GT_IND)) { - ContainBlockStoreAddress(blkNode, size, src->AsIndir()->Addr()); + ContainBlockStoreAddress(blkNode, size, src->AsIndir()->Addr(), src->AsIndir()); } - ContainBlockStoreAddress(blkNode, size, dstAddr); + ContainBlockStoreAddress(blkNode, size, dstAddr, nullptr); } else { @@ -328,8 +328,10 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) // blkNode - the block store node // size - the block size // addr - the address node to try to contain +// addrParent - the parent of addr, in case this is checking containment of the source address. // -void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr) +void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent) + { assert(blkNode->OperIs(GT_STORE_BLK) && (blkNode->gtBlkOpKind == GenTreeBlk::BlkOpKindUnroll)); assert(size < INT32_MAX); @@ -354,7 +356,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT return; } - if (!IsSafeToContainMem(blkNode, addr)) + if (!IsSafeToContainMem(blkNode, addrParent, addr)) { return; } diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 9fa8c0bf1c26bf..095cf91f61514a 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -373,7 +373,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) src->AsIntCon()->SetIconValue(fill); - ContainBlockStoreAddress(blkNode, size, dstAddr); + ContainBlockStoreAddress(blkNode, size, dstAddr, nullptr); } } else @@ -478,10 +478,10 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) if (src->OperIs(GT_IND)) { - ContainBlockStoreAddress(blkNode, size, src->AsIndir()->Addr()); + ContainBlockStoreAddress(blkNode, size, src->AsIndir()->Addr(), src->AsIndir()); } - ContainBlockStoreAddress(blkNode, size, dstAddr); + ContainBlockStoreAddress(blkNode, size, dstAddr, nullptr); } else { @@ -504,8 +504,9 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) // blkNode - the block store node // size - the block size // addr - the address node to try to contain +// addrParent - the parent of addr, in case this is checking containment of the source address. // -void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr) +void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent) { assert(blkNode->OperIs(GT_STORE_BLK) && (blkNode->gtBlkOpKind == GenTreeBlk::BlkOpKindUnroll)); assert(size < INT32_MAX); @@ -536,7 +537,7 @@ void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenT // Note that the parentNode is always the block node, even if we're dealing with the source address. // The source address is not directly used by the block node but by an IND node and that IND node is // always contained. - if (!IsSafeToContainMem(blkNode, addrMode)) + if (!IsSafeToContainMem(blkNode, addrParent, addrMode)) { return; } From 21eddd1441a92d2ac90fb1e4781e824e51ba4ff3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Wed, 23 Nov 2022 17:03:26 +0100 Subject: [PATCH 2/2] Update src/coreclr/jit/lowerarmarch.cpp Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> --- src/coreclr/jit/lowerarmarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 5901cba7a2870c..31b03274018892 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -658,7 +658,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) // blkNode - the block store node // size - the block size // addr - the address node to try to contain -// addrParent - the parent of addr, in case this is checking containment the source address. +// addrParent - the parent of addr, in case this is checking containment of the source address. // void Lowering::ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent) {