From b4307fdf89f76987d3bb6fbd8d5797925801fddc Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 04:32:07 +0100 Subject: [PATCH 01/12] Remove GenTreeBlk::BlkOpKindHelper --- src/coreclr/jit/codegen.h | 6 -- src/coreclr/jit/codegenarmarch.cpp | 68 ---------------- src/coreclr/jit/codegenloongarch64.cpp | 67 ---------------- src/coreclr/jit/codegenriscv64.cpp | 67 ---------------- src/coreclr/jit/codegenxarch.cpp | 55 ------------- src/coreclr/jit/gentree.cpp | 5 -- src/coreclr/jit/gentree.h | 3 - src/coreclr/jit/lower.cpp | 104 ++++++++++++++++++++++++- src/coreclr/jit/lower.h | 1 + src/coreclr/jit/lowerarmarch.cpp | 6 +- src/coreclr/jit/lowerloongarch64.cpp | 6 +- src/coreclr/jit/lowerriscv64.cpp | 6 +- src/coreclr/jit/lowerxarch.cpp | 26 +++---- src/coreclr/jit/lsraarmarch.cpp | 17 ---- src/coreclr/jit/lsrabuild.cpp | 12 --- src/coreclr/jit/lsraloongarch64.cpp | 17 ---- src/coreclr/jit/lsrariscv64.cpp | 17 ---- src/coreclr/jit/lsraxarch.cpp | 16 ---- 18 files changed, 125 insertions(+), 374 deletions(-) diff --git a/src/coreclr/jit/codegen.h b/src/coreclr/jit/codegen.h index afd9e42a9d2cdd..c36a6776a8cd9c 100644 --- a/src/coreclr/jit/codegen.h +++ b/src/coreclr/jit/codegen.h @@ -1182,9 +1182,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX void genCodeForCpObj(GenTreeBlk* cpObjNode); void genCodeForCpBlkRepMovs(GenTreeBlk* cpBlkNode); void genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode); -#ifndef TARGET_X86 - void genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode); -#endif void genCodeForPhysReg(GenTreePhysReg* tree); void genCodeForNullCheck(GenTreeIndir* tree); void genCodeForCmpXchg(GenTreeCmpXchg* tree); @@ -1257,9 +1254,6 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX #endif // FEATURE_PUT_STRUCT_ARG_STK void genCodeForStoreBlk(GenTreeBlk* storeBlkNode); -#ifndef TARGET_X86 - void genCodeForInitBlkHelper(GenTreeBlk* initBlkNode); -#endif void genCodeForInitBlkLoop(GenTreeBlk* initBlkNode); void genCodeForInitBlkRepStos(GenTreeBlk* initBlkNode); void genCodeForInitBlkUnroll(GenTreeBlk* initBlkNode); diff --git a/src/coreclr/jit/codegenarmarch.cpp b/src/coreclr/jit/codegenarmarch.cpp index 92e99471956f0d..8eb8d71b617a5e 100644 --- a/src/coreclr/jit/codegenarmarch.cpp +++ b/src/coreclr/jit/codegenarmarch.cpp @@ -1925,37 +1925,6 @@ void CodeGen::genCodeForIndir(GenTreeIndir* tree) genProduceReg(tree); } -//---------------------------------------------------------------------------------- -// genCodeForCpBlkHelper - Generate code for a CpBlk node by the means of the VM memcpy helper call -// -// Arguments: -// cpBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode) -{ - // Destination address goes in arg0, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(cpBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - if (cpBlkNode->IsVolatile()) - { - // issue a full memory barrier before a volatile CpBlk operation - instGen_MemoryBarrier(); - } - - genEmitHelperCall(CORINFO_HELP_MEMCPY, 0, EA_UNKNOWN); - - if (cpBlkNode->IsVolatile()) - { - // issue a load barrier after a volatile CpBlk operation - instGen_MemoryBarrier(BARRIER_LOAD_ONLY); - } -} - #ifdef TARGET_ARM64 // The following classes @@ -3218,31 +3187,6 @@ void CodeGen::genCodeForMemmove(GenTreeBlk* tree) #endif } -//------------------------------------------------------------------------ -// genCodeForInitBlkHelper - Generate code for an InitBlk node by the means of the VM memcpy helper call -// -// Arguments: -// initBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForInitBlkHelper(GenTreeBlk* initBlkNode) -{ - // Size goes in arg2, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(initBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - if (initBlkNode->IsVolatile()) - { - // issue a full memory barrier before a volatile initBlock Operation - instGen_MemoryBarrier(); - } - - genEmitHelperCall(CORINFO_HELP_MEMSET, 0, EA_UNKNOWN); -} - //------------------------------------------------------------------------ // genCodeForInitBlkLoop - Generate code for an InitBlk using an inlined for-loop. // It's needed for cases when size is too big to unroll and we're not allowed @@ -4630,18 +4574,6 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp) genCodeForInitBlkLoop(blkOp); break; - case GenTreeBlk::BlkOpKindHelper: - assert(!blkOp->gtBlkOpGcUnsafe); - if (isCopyBlk) - { - genCodeForCpBlkHelper(blkOp); - } - else - { - genCodeForInitBlkHelper(blkOp); - } - break; - case GenTreeBlk::BlkOpKindUnroll: case GenTreeBlk::BlkOpKindUnrollMemmove: if (isCopyBlk) diff --git a/src/coreclr/jit/codegenloongarch64.cpp b/src/coreclr/jit/codegenloongarch64.cpp index 41266917205a52..171ce81a4e5801 100644 --- a/src/coreclr/jit/codegenloongarch64.cpp +++ b/src/coreclr/jit/codegenloongarch64.cpp @@ -6130,37 +6130,6 @@ void CodeGen::genCodeForIndir(GenTreeIndir* tree) genProduceReg(tree); } -//---------------------------------------------------------------------------------- -// genCodeForCpBlkHelper - Generate code for a CpBlk node by the means of the VM memcpy helper call -// -// Arguments: -// cpBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode) -{ - // Destination address goes in arg0, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(cpBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - if (cpBlkNode->IsVolatile()) - { - // issue a full memory barrier before a volatile CpBlk operation - instGen_MemoryBarrier(); - } - - genEmitHelperCall(CORINFO_HELP_MEMCPY, 0, EA_UNKNOWN); - - if (cpBlkNode->IsVolatile()) - { - // issue a INS_BARRIER_RMB after a volatile CpBlk operation - instGen_MemoryBarrier(BARRIER_FULL); - } -} - //---------------------------------------------------------------------------------- // genCodeForCpBlkUnroll: Generates CpBlk code by performing a loop unroll // @@ -6343,31 +6312,6 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode) } } -//------------------------------------------------------------------------ -// genCodeForInitBlkHelper - Generate code for an InitBlk node by the means of the VM memcpy helper call -// -// Arguments: -// initBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForInitBlkHelper(GenTreeBlk* initBlkNode) -{ - // Size goes in arg2, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(initBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - if (initBlkNode->IsVolatile()) - { - // issue a full memory barrier before a volatile initBlock Operation - instGen_MemoryBarrier(); - } - - genEmitHelperCall(CORINFO_HELP_MEMSET, 0, EA_UNKNOWN); -} - //------------------------------------------------------------------------ // genCodeForInitBlkLoop - Generate code for an InitBlk using an inlined for-loop. // It's needed for cases when size is too big to unroll and we're not allowed @@ -7332,17 +7276,6 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp) genCodeForInitBlkLoop(blkOp); break; - case GenTreeBlk::BlkOpKindHelper: - if (isCopyBlk) - { - genCodeForCpBlkHelper(blkOp); - } - else - { - genCodeForInitBlkHelper(blkOp); - } - break; - case GenTreeBlk::BlkOpKindUnroll: if (isCopyBlk) { diff --git a/src/coreclr/jit/codegenriscv64.cpp b/src/coreclr/jit/codegenriscv64.cpp index a468c026c22cbd..7cb988cb1be12e 100644 --- a/src/coreclr/jit/codegenriscv64.cpp +++ b/src/coreclr/jit/codegenriscv64.cpp @@ -6166,37 +6166,6 @@ void CodeGen::genCodeForIndir(GenTreeIndir* tree) genProduceReg(tree); } -//---------------------------------------------------------------------------------- -// genCodeForCpBlkHelper - Generate code for a CpBlk node by the means of the VM memcpy helper call -// -// Arguments: -// cpBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode) -{ - // Destination address goes in arg0, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(cpBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - if (cpBlkNode->IsVolatile()) - { - // issue a full memory barrier before a volatile CpBlk operation - instGen_MemoryBarrier(); - } - - genEmitHelperCall(CORINFO_HELP_MEMCPY, 0, EA_UNKNOWN); - - if (cpBlkNode->IsVolatile()) - { - // issue a INS_BARRIER_RMB after a volatile CpBlk operation - instGen_MemoryBarrier(BARRIER_FULL); - } -} - //---------------------------------------------------------------------------------- // genCodeForCpBlkUnroll: Generates CpBlk code by performing a loop unroll // @@ -6434,31 +6403,6 @@ void CodeGen::genCodeForInitBlkLoop(GenTreeBlk* initBlkNode) } } -//------------------------------------------------------------------------ -// genCodeForInitBlkHelper - Generate code for an InitBlk node by the means of the VM memcpy helper call -// -// Arguments: -// initBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForInitBlkHelper(GenTreeBlk* initBlkNode) -{ - // Size goes in arg2, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(initBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - if (initBlkNode->IsVolatile()) - { - // issue a full memory barrier before a volatile initBlock Operation - instGen_MemoryBarrier(); - } - - genEmitHelperCall(CORINFO_HELP_MEMSET, 0, EA_UNKNOWN); -} - //------------------------------------------------------------------------ // genCall: Produce code for a GT_CALL node // @@ -7328,17 +7272,6 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp) genCodeForInitBlkLoop(blkOp); break; - case GenTreeBlk::BlkOpKindHelper: - if (isCopyBlk) - { - genCodeForCpBlkHelper(blkOp); - } - else - { - genCodeForInitBlkHelper(blkOp); - } - break; - case GenTreeBlk::BlkOpKindUnroll: if (isCopyBlk) { diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index b877262b1583d8..c015b876ff8e42 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -3070,19 +3070,6 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* storeBlkNode) genCodeForInitBlkLoop(storeBlkNode); break; -#ifdef TARGET_AMD64 - case GenTreeBlk::BlkOpKindHelper: - assert(!storeBlkNode->gtBlkOpGcUnsafe); - if (isCopyBlk) - { - genCodeForCpBlkHelper(storeBlkNode); - } - else - { - genCodeForInitBlkHelper(storeBlkNode); - } - break; -#endif // TARGET_AMD64 case GenTreeBlk::BlkOpKindRepInstr: #ifndef JIT32_GCENCODER assert(!storeBlkNode->gtBlkOpGcUnsafe); @@ -3402,27 +3389,6 @@ void CodeGen::genCodeForInitBlkLoop(GenTreeBlk* initBlkNode) } } -#ifdef TARGET_AMD64 -//------------------------------------------------------------------------ -// genCodeForInitBlkHelper - Generate code for an InitBlk node by the means of the VM memcpy helper call -// -// Arguments: -// initBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForInitBlkHelper(GenTreeBlk* initBlkNode) -{ - // Destination address goes in arg0, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(initBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - genEmitHelperCall(CORINFO_HELP_MEMSET, 0, EA_UNKNOWN); -} -#endif // TARGET_AMD64 - #ifdef FEATURE_PUT_STRUCT_ARG_STK // Generate code for a load from some address + offset // base: tree node which can be either a local or an indir @@ -4310,27 +4276,6 @@ void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode) gcInfo.gcMarkRegSetNpt(RBM_RDI); } -#ifdef TARGET_AMD64 -//---------------------------------------------------------------------------------- -// genCodeForCpBlkHelper - Generate code for a CpBlk node by the means of the VM memcpy helper call -// -// Arguments: -// cpBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] -// -// Preconditions: -// The register assignments have been set appropriately. -// This is validated by genConsumeBlockOp(). -// -void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode) -{ - // Destination address goes in arg0, source address goes in arg1, and size goes in arg2. - // genConsumeBlockOp takes care of this for us. - genConsumeBlockOp(cpBlkNode, REG_ARG_0, REG_ARG_1, REG_ARG_2); - - genEmitHelperCall(CORINFO_HELP_MEMCPY, 0, EA_UNKNOWN); -} -#endif // TARGET_AMD64 - // generate code do a switch statement based on a table of ip-relative offsets void CodeGen::genTableBasedSwitch(GenTree* treeNode) { diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index ad68efef307b77..836d60ff65f2cb 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -12703,11 +12703,6 @@ void Compiler::gtDispTree(GenTree* tree, case GenTreeBlk::BlkOpKindUnrollMemmove: printf(" (Memmove)"); break; -#ifndef TARGET_X86 - case GenTreeBlk::BlkOpKindHelper: - printf(" (Helper)"); - break; -#endif case GenTreeBlk::BlkOpKindLoop: printf(" (Loop)"); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 1bd7ce27004fe6..b1744720d05bc5 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -7392,9 +7392,6 @@ struct GenTreeBlk : public GenTreeIndir #ifdef TARGET_XARCH BlkOpKindCpObjRepInstr, #endif -#ifndef TARGET_X86 - BlkOpKindHelper, -#endif #ifdef TARGET_XARCH BlkOpKindRepInstr, #endif diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 591db3a78a22c0..c59dd82541fca5 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7844,6 +7844,106 @@ void Lowering::ContainCheckBitCast(GenTree* node) } } +void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) +{ + // We shouldn't be using helper calls for blocks on heap containing GC pointers. + // due to atomicity guarantees. + assert(!blkNode->IsZeroingGcPointersOnHeap()); + + LIR::Use use; + assert(!BlockRange().TryGetUse(blkNode, &use)); + + GenTree* dest = blkNode->Addr(); + GenTree* data = blkNode->Data(); + GenTree* size; + + if (blkNode->OperIsInitBlkOp()) + { + if (data->OperIsInitVal()) + { + BlockRange().Remove(data); + data = data->gtGetOp1(); + } + } + else + { + if (data->OperIs(GT_IND)) + { + BlockRange().Remove(data); + data = data->AsIndir()->Addr(); + } + + if (varTypeIsStruct(data)) + { + const unsigned lclNum = data->AsLclVarCommon()->GetLclNum(); + GenTreeLclFld* addr = comp->gtNewLclAddrNode(lclNum, data->AsLclVarCommon()->GetLclOffs(), TYP_BYREF); + BlockRange().InsertAfter(data, addr); + BlockRange().Remove(data); + data = addr; + } + } + + if (blkNode->OperIs(GT_STORE_DYN_BLK)) + { + size = blkNode->AsStoreDynBlk()->gtDynamicSize; + } + else + { + size = comp->gtNewIconNode(blkNode->Size(), TYP_I_IMPL); + BlockRange().InsertBefore(data, size); + } + + GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); + GenTree* dataPlaceholder = comp->gtNewZeroConNode(data->TypeGet()); + GenTree* sizePlaceholder = comp->gtNewZeroConNode(size->TypeGet()); + + CorInfoHelpFunc helper = blkNode->OperIsInitBlkOp() ? CORINFO_HELP_MEMSET : CORINFO_HELP_MEMCPY; + GenTreeCall* call = comp->gtNewHelperCallNode(helper, TYP_VOID, destPlaceholder, dataPlaceholder, sizePlaceholder); + comp->fgMorphTree(call); + + LIR::Range range = LIR::SeqTree(comp, call); + GenTree* rangeStart = range.FirstNode(); + GenTree* rangeEnd = range.LastNode(); + + BlockRange().InsertBefore(blkNode, std::move(range)); + blkNode->gtBashToNOP(); + + LIR::Use destUse; + LIR::Use dataUse; + LIR::Use sizeUse; + BlockRange().TryGetUse(destPlaceholder, &destUse); + BlockRange().TryGetUse(dataPlaceholder, &dataUse); + BlockRange().TryGetUse(sizePlaceholder, &sizeUse); + destUse.ReplaceWith(dest); + dataUse.ReplaceWith(data); + sizeUse.ReplaceWith(size); + destPlaceholder->SetUnusedValue(); + dataPlaceholder->SetUnusedValue(); + sizePlaceholder->SetUnusedValue(); + + LowerRange(rangeStart, rangeEnd); + + // Finally move all GT_PUTARG_* nodes + for (CallArg& arg : call->gtArgs.EarlyArgs()) + { + GenTree* node = arg.GetEarlyNode(); + // Non-value nodes in early args are setup nodes for late args. + if (node->IsValue()) + { + assert(node->OperIsPutArg() || node->OperIsFieldList()); + MoveCFGCallArg(call, node); + } + } + for (CallArg& arg : call->gtArgs.LateArgs()) + { + GenTree* node = arg.GetLateNode(); + assert(node->OperIsPutArg() || node->OperIsFieldList()); + MoveCFGCallArg(call, node); + } + + // TODO: emit memory barriers for volatile block stores (volatile. cpblk/initblk) +} + struct StoreCoalescingData { var_types targetType; @@ -8503,7 +8603,7 @@ bool Lowering::TryMakeIndirsAdjacent(GenTreeIndir* prevIndir, GenTreeIndir* indi // We can reorder indirs with some calls, but introducing a LIR edge // that spans a call can introduce spills (or callee-saves). - if (cur->IsCall() || (cur->OperIsStoreBlk() && (cur->AsBlk()->gtBlkOpKind == GenTreeBlk::BlkOpKindHelper))) + if (cur->IsCall()) { JITDUMP(" ..but they are separated by node [%06u] that kills registers\n", Compiler::dspTreeID(cur)); return false; @@ -8840,7 +8940,7 @@ void Lowering::LowerLclHeap(GenTree* node) GenTreeBlk(GT_STORE_BLK, TYP_STRUCT, heapLcl, zero, comp->typGetBlkLayout((unsigned)alignedSize)); storeBlk->gtFlags |= (GTF_IND_UNALIGNED | GTF_ASG | GTF_EXCEPT | GTF_GLOB_REF); BlockRange().InsertAfter(use.Def(), heapLcl, zero, storeBlk); - LowerNode(storeBlk); + return; } else { diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index d35738c944dcf2..6b5cae3bed3799 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -331,6 +331,7 @@ class Lowering final : public Phase GenTree* LowerSignedDivOrMod(GenTree* node); void LowerBlockStore(GenTreeBlk* blkNode); void LowerBlockStoreCommon(GenTreeBlk* blkNode); + void LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode); void LowerLclHeap(GenTree* node); void ContainBlockStoreAddress(GenTreeBlk* blkNode, unsigned size, GenTree* addr, GenTree* addrParent); void LowerPutArgStkOrSplit(GenTreePutArgStk* putArgNode); diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 5c0acbbdc40115..4cc7144ad15d39 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -634,7 +634,8 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) } else { - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); + return; } } else @@ -686,8 +687,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) else { assert(blkNode->OperIs(GT_STORE_BLK, GT_STORE_DYN_BLK)); - - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); } } } diff --git a/src/coreclr/jit/lowerloongarch64.cpp b/src/coreclr/jit/lowerloongarch64.cpp index 5110442eda10d1..507ef59256ca4c 100644 --- a/src/coreclr/jit/lowerloongarch64.cpp +++ b/src/coreclr/jit/lowerloongarch64.cpp @@ -334,7 +334,8 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) } else { - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); + return; } } else @@ -387,8 +388,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) else { assert(blkNode->OperIs(GT_STORE_BLK, GT_STORE_DYN_BLK)); - - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); } } } diff --git a/src/coreclr/jit/lowerriscv64.cpp b/src/coreclr/jit/lowerriscv64.cpp index 4f60458fd25161..405b9707c41145 100644 --- a/src/coreclr/jit/lowerriscv64.cpp +++ b/src/coreclr/jit/lowerriscv64.cpp @@ -282,7 +282,8 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) } else { - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); + return; } } else @@ -334,8 +335,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) else { assert(blkNode->OperIs(GT_STORE_BLK, GT_STORE_DYN_BLK)); - - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); } } } diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 6709a57a49e8d7..ea4db9fedbac46 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -407,14 +407,20 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) else { TOO_BIG_TO_UNROLL: + if (blkNode->IsZeroingGcPointersOnHeap()) + { + blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindLoop; + } + else + { #ifdef TARGET_AMD64 - blkNode->gtBlkOpKind = - blkNode->IsZeroingGcPointersOnHeap() ? GenTreeBlk::BlkOpKindLoop : GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); + return; #else - // TODO-X86-CQ: Investigate whether a helper call would be beneficial on x86 - blkNode->gtBlkOpKind = - blkNode->IsZeroingGcPointersOnHeap() ? GenTreeBlk::BlkOpKindLoop : GenTreeBlk::BlkOpKindRepInstr; + // TODO-X86-CQ: Investigate whether a helper call would be beneficial on x86 + blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindRepInstr; #endif + } } } else @@ -513,7 +519,8 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) assert(blkNode->OperIs(GT_STORE_BLK, GT_STORE_DYN_BLK)); #ifdef TARGET_AMD64 - blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindHelper; + LowerBlockStoreAsHelperCall(blkNode); + return; #else // TODO-X86-CQ: Investigate whether a helper call would be beneficial on x86 blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindRepInstr; @@ -522,13 +529,6 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) } assert(blkNode->gtBlkOpKind != GenTreeBlk::BlkOpKindInvalid); - -#ifndef TARGET_X86 - if ((MIN_ARG_AREA_FOR_CALL > 0) && (blkNode->gtBlkOpKind == GenTreeBlk::BlkOpKindHelper)) - { - RequireOutgoingArgSpace(blkNode, MIN_ARG_AREA_FOR_CALL); - } -#endif } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/lsraarmarch.cpp b/src/coreclr/jit/lsraarmarch.cpp index a0f27ba65b3058..ad112a817220f8 100644 --- a/src/coreclr/jit/lsraarmarch.cpp +++ b/src/coreclr/jit/lsraarmarch.cpp @@ -644,13 +644,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) buildInternalIntRegisterDefForNode(blkNode, availableIntRegs); break; - case GenTreeBlk::BlkOpKindHelper: - assert(!src->isContained()); - dstAddrRegMask = RBM_ARG_0; - srcRegMask = RBM_ARG_1; - sizeRegMask = RBM_ARG_2; - break; - default: unreached(); } @@ -783,16 +776,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) } break; - case GenTreeBlk::BlkOpKindHelper: - dstAddrRegMask = RBM_ARG_0; - if (srcAddrOrFill != nullptr) - { - assert(!srcAddrOrFill->isContained()); - srcRegMask = RBM_ARG_1; - } - sizeRegMask = RBM_ARG_2; - break; - default: unreached(); } diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index 0c1d3f74475c8d..3b9ec7f388aec1 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -907,18 +907,6 @@ regMaskTP LinearScan::getKillSetForBlockStore(GenTreeBlk* blkNode) killMask = compiler->compHelperCallKillSet(CORINFO_HELP_ASSIGN_BYREF); break; -#ifndef TARGET_X86 - case GenTreeBlk::BlkOpKindHelper: - if (isCopyBlk) - { - killMask = compiler->compHelperCallKillSet(CORINFO_HELP_MEMCPY); - } - else - { - killMask = compiler->compHelperCallKillSet(CORINFO_HELP_MEMSET); - } - break; -#endif #ifdef TARGET_XARCH case GenTreeBlk::BlkOpKindRepInstr: if (isCopyBlk) diff --git a/src/coreclr/jit/lsraloongarch64.cpp b/src/coreclr/jit/lsraloongarch64.cpp index 67b27aa51300c8..b6e3b53d63ee59 100644 --- a/src/coreclr/jit/lsraloongarch64.cpp +++ b/src/coreclr/jit/lsraloongarch64.cpp @@ -1104,13 +1104,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) buildInternalIntRegisterDefForNode(blkNode, availableIntRegs); break; - case GenTreeBlk::BlkOpKindHelper: - assert(!src->isContained()); - dstAddrRegMask = RBM_ARG_0; - srcRegMask = RBM_ARG_1; - sizeRegMask = RBM_ARG_2; - break; - default: unreached(); } @@ -1159,16 +1152,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) buildInternalIntRegisterDefForNode(blkNode); break; - case GenTreeBlk::BlkOpKindHelper: - dstAddrRegMask = RBM_ARG_0; - if (srcAddrOrFill != nullptr) - { - assert(!srcAddrOrFill->isContained()); - srcRegMask = RBM_ARG_1; - } - sizeRegMask = RBM_ARG_2; - break; - default: unreached(); } diff --git a/src/coreclr/jit/lsrariscv64.cpp b/src/coreclr/jit/lsrariscv64.cpp index ec4ca4a34972b8..0188c3c0627116 100644 --- a/src/coreclr/jit/lsrariscv64.cpp +++ b/src/coreclr/jit/lsrariscv64.cpp @@ -1260,13 +1260,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) buildInternalIntRegisterDefForNode(blkNode, availableIntRegs); break; - case GenTreeBlk::BlkOpKindHelper: - assert(!src->isContained()); - dstAddrRegMask = RBM_ARG_0; - srcRegMask = RBM_ARG_1; - sizeRegMask = RBM_ARG_2; - break; - default: unreached(); } @@ -1315,16 +1308,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) buildInternalIntRegisterDefForNode(blkNode); break; - case GenTreeBlk::BlkOpKindHelper: - dstAddrRegMask = RBM_ARG_0; - if (srcAddrOrFill != nullptr) - { - assert(!srcAddrOrFill->isContained()); - srcRegMask = RBM_ARG_1; - } - sizeRegMask = RBM_ARG_2; - break; - default: unreached(); } diff --git a/src/coreclr/jit/lsraxarch.cpp b/src/coreclr/jit/lsraxarch.cpp index cb2b82d5d8ce92..41121ee9bed28e 100644 --- a/src/coreclr/jit/lsraxarch.cpp +++ b/src/coreclr/jit/lsraxarch.cpp @@ -1434,14 +1434,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) buildInternalIntRegisterDefForNode(blkNode, availableIntRegs); break; -#ifdef TARGET_AMD64 - case GenTreeBlk::BlkOpKindHelper: - dstAddrRegMask = RBM_ARG_0; - srcRegMask = RBM_ARG_1; - sizeRegMask = RBM_ARG_2; - break; -#endif - default: unreached(); } @@ -1562,14 +1554,6 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode) sizeRegMask = RBM_RCX; break; -#ifdef TARGET_AMD64 - case GenTreeBlk::BlkOpKindHelper: - dstAddrRegMask = RBM_ARG_0; - srcRegMask = RBM_ARG_1; - sizeRegMask = RBM_ARG_2; - break; -#endif - default: unreached(); } From 3fb740e2daa0820dd7c6d8d6b89029be92d50b98 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 15:27:55 +0100 Subject: [PATCH 02/12] Address feedback --- src/coreclr/jit/compiler.h | 2 + src/coreclr/jit/gentree.cpp | 20 ++++++ src/coreclr/jit/importercalls.cpp | 11 +--- src/coreclr/jit/lower.cpp | 103 ++++++++++++++++++++---------- src/coreclr/jit/lower.h | 1 + 5 files changed, 93 insertions(+), 44 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index c850b8f76176a2..a60283af6d600b 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -3311,6 +3311,8 @@ class Compiler #endif #endif // FEATURE_HW_INTRINSICS + GenTree* gtNewMemoryBarrier(bool loadOnly = false); + GenTree* gtNewMustThrowException(unsigned helper, var_types type, CORINFO_CLASS_HANDLE clsHnd); GenTreeLclFld* gtNewLclFldNode(unsigned lnum, var_types type, unsigned offset); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 836d60ff65f2cb..ee6206a162cb5e 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -8651,6 +8651,26 @@ GenTreeBlk* Compiler::gtNewBlkIndir(ClassLayout* layout, GenTree* addr, GenTreeF return blkNode; } +//------------------------------------------------------------------------ +// gtNewMemoryBarrier: Create a memory barrier node +// +// Arguments: +// loadOnly - relaxes the full memory barrier to be load-only +// +// Return Value: +// The created GT_MEMORYBARRIER node. +// +GenTree* Compiler::gtNewMemoryBarrier(bool loadOnly) +{ + GenTree* tree = new (this, GT_MEMORYBARRIER) GenTree(GT_MEMORYBARRIER, TYP_VOID); + tree->gtFlags |= GTF_GLOB_REF | GTF_ASG; + if (loadOnly) + { + tree->gtFlags |= GTF_MEMORYBARRIER_LOAD; + } + return tree; +} + //------------------------------------------------------------------------------ // gtNewIndir : Create an indirection node. // diff --git a/src/coreclr/jit/importercalls.cpp b/src/coreclr/jit/importercalls.cpp index b263285c42b545..50b6348d1679df 100644 --- a/src/coreclr/jit/importercalls.cpp +++ b/src/coreclr/jit/importercalls.cpp @@ -3526,18 +3526,9 @@ GenTree* Compiler::impIntrinsic(GenTree* newobjThis, case NI_System_Threading_Interlocked_ReadMemoryBarrier: { assert(sig->numArgs == 0); - - GenTree* op1 = new (this, GT_MEMORYBARRIER) GenTree(GT_MEMORYBARRIER, TYP_VOID); - op1->gtFlags |= GTF_GLOB_REF | GTF_ASG; - // On XARCH `NI_System_Threading_Interlocked_ReadMemoryBarrier` fences need not be emitted. // However, we still need to capture the effect on reordering. - if (ni == NI_System_Threading_Interlocked_ReadMemoryBarrier) - { - op1->gtFlags |= GTF_MEMORYBARRIER_LOAD; - } - - retNode = op1; + retNode = gtNewMemoryBarrier(ni == NI_System_Threading_Interlocked_ReadMemoryBarrier); break; } diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index c59dd82541fca5..6438e79d7ea049 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -3098,23 +3098,7 @@ void Lowering::LowerCFGCall(GenTreeCall* call) LowerNode(regNode); // Finally move all GT_PUTARG_* nodes - for (CallArg& arg : call->gtArgs.EarlyArgs()) - { - GenTree* node = arg.GetEarlyNode(); - // Non-value nodes in early args are setup nodes for late args. - if (node->IsValue()) - { - assert(node->OperIsPutArg() || node->OperIsFieldList()); - MoveCFGCallArg(call, node); - } - } - - for (CallArg& arg : call->gtArgs.LateArgs()) - { - GenTree* node = arg.GetLateNode(); - assert(node->OperIsPutArg() || node->OperIsFieldList()); - MoveCFGCallArg(call, node); - } + MoveCFGCallArgs(call); break; } case CFGCallKind::Dispatch: @@ -3261,6 +3245,38 @@ void Lowering::MoveCFGCallArg(GenTreeCall* call, GenTree* node) BlockRange().InsertBefore(call, node); } +//------------------------------------------------------------------------ +// MoveCFGCallArgs: Given a call that will be CFG transformed using the +// validate+call scheme, move all GT_PUTARG_* or GT_FIELD_LIST nodes right before the call. +// +// Arguments: +// call - The call that is being CFG transformed +// +// Remarks: +// See comments in MoveCFGCallArg for more details. +// +void Lowering::MoveCFGCallArgs(GenTreeCall* call) +{ + // Finally move all GT_PUTARG_* nodes + for (CallArg& arg : call->gtArgs.EarlyArgs()) + { + GenTree* node = arg.GetEarlyNode(); + // Non-value nodes in early args are setup nodes for late args. + if (node->IsValue()) + { + assert(node->OperIsPutArg() || node->OperIsFieldList()); + MoveCFGCallArg(call, node); + } + } + + for (CallArg& arg : call->gtArgs.LateArgs()) + { + GenTree* node = arg.GetLateNode(); + assert(node->OperIsPutArg() || node->OperIsFieldList()); + MoveCFGCallArg(call, node); + } +} + #ifndef TARGET_64BIT //------------------------------------------------------------------------ // Lowering::DecomposeLongCompare: Decomposes a TYP_LONG compare node. @@ -7844,6 +7860,12 @@ void Lowering::ContainCheckBitCast(GenTree* node) } } +//------------------------------------------------------------------------ +// LowerBlockStoreAsHelperCall: Lower a block store node as a memset/memcpy call +// +// Arguments: +// blkNode - The block store node to lower +// void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) { // We shouldn't be using helper calls for blocks on heap containing GC pointers. @@ -7853,12 +7875,16 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) LIR::Use use; assert(!BlockRange().TryGetUse(blkNode, &use)); + const bool isVolatile = blkNode->IsVolatile(); + GenTree* dest = blkNode->Addr(); GenTree* data = blkNode->Data(); GenTree* size; + // Is it Memset ... if (blkNode->OperIsInitBlkOp()) { + // Drop GT_INIT_VAL nodes if (data->OperIsInitVal()) { BlockRange().Remove(data); @@ -7867,6 +7893,10 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) } else { + // ... or Memcpy? + assert(data->OperIs(GT_IND, GT_LCL_VAR, GT_LCL_FLD)); + + // Drop GT_IND nodes if (data->OperIs(GT_IND)) { BlockRange().Remove(data); @@ -7875,6 +7905,7 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) if (varTypeIsStruct(data)) { + // If it's a struct local, take its address const unsigned lclNum = data->AsLclVarCommon()->GetLclNum(); GenTreeLclFld* addr = comp->gtNewLclAddrNode(lclNum, data->AsLclVarCommon()->GetLclOffs(), TYP_BYREF); BlockRange().InsertAfter(data, addr); @@ -7885,14 +7916,17 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) if (blkNode->OperIs(GT_STORE_DYN_BLK)) { + // Size is not a constant size = blkNode->AsStoreDynBlk()->gtDynamicSize; } else { + // Size is a constant size = comp->gtNewIconNode(blkNode->Size(), TYP_I_IMPL); BlockRange().InsertBefore(data, size); } + // A hacky way to safely call fgMorphTree in Lower GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); GenTree* dataPlaceholder = comp->gtNewZeroConNode(data->TypeGet()); GenTree* sizePlaceholder = comp->gtNewZeroConNode(size->TypeGet()); @@ -7924,24 +7958,26 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) LowerRange(rangeStart, rangeEnd); // Finally move all GT_PUTARG_* nodes - for (CallArg& arg : call->gtArgs.EarlyArgs()) - { - GenTree* node = arg.GetEarlyNode(); - // Non-value nodes in early args are setup nodes for late args. - if (node->IsValue()) - { - assert(node->OperIsPutArg() || node->OperIsFieldList()); - MoveCFGCallArg(call, node); - } - } - for (CallArg& arg : call->gtArgs.LateArgs()) + // Re-use the existing logic for CFG call args here + MoveCFGCallArgs(call); + + BlockRange().Remove(destPlaceholder); + BlockRange().Remove(dataPlaceholder); + BlockRange().Remove(sizePlaceholder); + + // Wrap with memory barriers on weak memory models + // if the block store was volatile +#ifndef TARGET_XARCH + if (isVolatile) { - GenTree* node = arg.GetLateNode(); - assert(node->OperIsPutArg() || node->OperIsFieldList()); - MoveCFGCallArg(call, node); + GenTree* firstBarrier = comp->gtNewMemoryBarrier(); + GenTree* secondBarrier = comp->gtNewMemoryBarrier(/*loadOnly*/ true); + BlockRange().InsertBefore(call, firstBarrier); + BlockRange().InsertAfter(call, secondBarrier); + LowerNode(firstBarrier); + LowerNode(secondBarrier); } - - // TODO: emit memory barriers for volatile block stores (volatile. cpblk/initblk) +#endif } struct StoreCoalescingData @@ -8940,7 +8976,6 @@ void Lowering::LowerLclHeap(GenTree* node) GenTreeBlk(GT_STORE_BLK, TYP_STRUCT, heapLcl, zero, comp->typGetBlkLayout((unsigned)alignedSize)); storeBlk->gtFlags |= (GTF_IND_UNALIGNED | GTF_ASG | GTF_EXCEPT | GTF_GLOB_REF); BlockRange().InsertAfter(use.Def(), heapLcl, zero, storeBlk); - return; } else { diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 6b5cae3bed3799..1b0c0f8a1725ae 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -141,6 +141,7 @@ class Lowering final : public Phase bool LowerCallMemmove(GenTreeCall* call, GenTree** next); bool LowerCallMemcmp(GenTreeCall* call, GenTree** next); void LowerCFGCall(GenTreeCall* call); + void MoveCFGCallArgs(GenTreeCall* call); void MoveCFGCallArg(GenTreeCall* call, GenTree* node); #ifndef TARGET_64BIT GenTree* DecomposeLongCompare(GenTree* cmp); From 1d3a82f5f49f5865542987e6feadafe4b643fc0a Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 15:50:36 +0100 Subject: [PATCH 03/12] formatting --- src/coreclr/jit/lower.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 6438e79d7ea049..d49ca005f4f616 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7965,12 +7965,12 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) BlockRange().Remove(dataPlaceholder); BlockRange().Remove(sizePlaceholder); - // Wrap with memory barriers on weak memory models - // if the block store was volatile +// Wrap with memory barriers on weak memory models +// if the block store was volatile #ifndef TARGET_XARCH if (isVolatile) { - GenTree* firstBarrier = comp->gtNewMemoryBarrier(); + GenTree* firstBarrier = comp->gtNewMemoryBarrier(); GenTree* secondBarrier = comp->gtNewMemoryBarrier(/*loadOnly*/ true); BlockRange().InsertBefore(call, firstBarrier); BlockRange().InsertAfter(call, secondBarrier); From a28b2139a0131a513cabf751a5228e290d5c972f Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 20:43:51 +0100 Subject: [PATCH 04/12] Add Jakob's suggestion --- src/coreclr/jit/codegencommon.cpp | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/codegencommon.cpp b/src/coreclr/jit/codegencommon.cpp index 825837fe45ef50..3c001249c9a054 100644 --- a/src/coreclr/jit/codegencommon.cpp +++ b/src/coreclr/jit/codegencommon.cpp @@ -6548,10 +6548,19 @@ void CodeGen::genDefinePendingCallLabel(GenTreeCall* call) // For certain indirect calls we may introduce helper calls before that we need to skip: // - CFG may introduce a call to the validator first // - Generic virtual methods may compute the target dynamically through a separate helper call - if (call->IsHelperCall(compiler, CORINFO_HELP_VALIDATE_INDIRECT_CALL) || - call->IsHelperCall(compiler, CORINFO_HELP_VIRTUAL_FUNC_PTR)) + // - memset/memcpy helper calls emitted for GT_STORE_DYN_BLK/GT_STORE_BLK + if (call->IsHelperCall()) { - return; + switch (compiler->eeGetHelperNum(call->gtCallMethHnd)) + { + case CORINFO_HELP_VALIDATE_INDIRECT_CALL: + case CORINFO_HELP_VIRTUAL_FUNC_PTR: + case CORINFO_HELP_MEMSET: + case CORINFO_HELP_MEMCPY: + return; + default: + break; + } } genDefineInlineTempLabel(genPendingCallLabel); From a8b0b829393d4b1a63ad33eab720393d7ed97b87 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 20 Feb 2024 21:08:16 +0100 Subject: [PATCH 05/12] Update src/coreclr/jit/lower.cpp Co-authored-by: Jakob Botsch Nielsen --- src/coreclr/jit/lower.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d49ca005f4f616..a6be27efef3c30 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7928,8 +7928,8 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) // A hacky way to safely call fgMorphTree in Lower GenTree* destPlaceholder = comp->gtNewZeroConNode(dest->TypeGet()); - GenTree* dataPlaceholder = comp->gtNewZeroConNode(data->TypeGet()); - GenTree* sizePlaceholder = comp->gtNewZeroConNode(size->TypeGet()); + GenTree* dataPlaceholder = comp->gtNewZeroConNode(genActualType(data)); + GenTree* sizePlaceholder = comp->gtNewZeroConNode(genActualType(size)); CorInfoHelpFunc helper = blkNode->OperIsInitBlkOp() ? CORINFO_HELP_MEMSET : CORINFO_HELP_MEMCPY; GenTreeCall* call = comp->gtNewHelperCallNode(helper, TYP_VOID, destPlaceholder, dataPlaceholder, sizePlaceholder); From ab62c5f31925ad560e320cbdb45058e6169cd37d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 21:16:21 +0100 Subject: [PATCH 06/12] Address feedback --- src/coreclr/jit/lower.cpp | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a6be27efef3c30..d8ca1a90285be5 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7894,23 +7894,19 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) else { // ... or Memcpy? - assert(data->OperIs(GT_IND, GT_LCL_VAR, GT_LCL_FLD)); - - // Drop GT_IND nodes if (data->OperIs(GT_IND)) { + // Drop GT_IND nodes BlockRange().Remove(data); data = data->AsIndir()->Addr(); } - - if (varTypeIsStruct(data)) + else { - // If it's a struct local, take its address - const unsigned lclNum = data->AsLclVarCommon()->GetLclNum(); - GenTreeLclFld* addr = comp->gtNewLclAddrNode(lclNum, data->AsLclVarCommon()->GetLclOffs(), TYP_BYREF); - BlockRange().InsertAfter(data, addr); - BlockRange().Remove(data); - data = addr; + assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); + + // Convert local to LCL_ADDR + data->ChangeOper(GT_LCL_ADDR); + data->ChangeType(TYP_BYREF); } } @@ -7933,7 +7929,7 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) CorInfoHelpFunc helper = blkNode->OperIsInitBlkOp() ? CORINFO_HELP_MEMSET : CORINFO_HELP_MEMCPY; GenTreeCall* call = comp->gtNewHelperCallNode(helper, TYP_VOID, destPlaceholder, dataPlaceholder, sizePlaceholder); - comp->fgMorphTree(call); + comp->fgMorphArgs(call); LIR::Range range = LIR::SeqTree(comp, call); GenTree* rangeStart = range.FirstNode(); From 8765581bd26ade58021a23876f516da72fe70745 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 21:17:07 +0100 Subject: [PATCH 07/12] TYP_I_IMPL --- src/coreclr/jit/lower.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d8ca1a90285be5..a4aeb304f932d7 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7906,7 +7906,7 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) // Convert local to LCL_ADDR data->ChangeOper(GT_LCL_ADDR); - data->ChangeType(TYP_BYREF); + data->ChangeType(TYP_I_IMPL); } } From 76b8d8371a474516479cac0b6de8682cfb4171b4 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 21:25:15 +0100 Subject: [PATCH 08/12] ClearContainment --- src/coreclr/jit/lower.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a4aeb304f932d7..1a22a0a55fca8b 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7903,10 +7903,9 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) else { assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); - - // Convert local to LCL_ADDR data->ChangeOper(GT_LCL_ADDR); - data->ChangeType(TYP_I_IMPL); + data->ChangeType(TYP_BYREF); + data->ClearContained(); } } From 890f77fa668951be36a9fecf594ab4508dfbcf08 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Tue, 20 Feb 2024 21:26:10 +0100 Subject: [PATCH 09/12] Oops --- src/coreclr/jit/lower.cpp | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 1a22a0a55fca8b..b78c578a4d5672 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7903,8 +7903,10 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) else { assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); + + // Convert local to LCL_ADDR data->ChangeOper(GT_LCL_ADDR); - data->ChangeType(TYP_BYREF); + data->ChangeType(TYP_I_IMPL); data->ClearContained(); } } From bb380afd149aff4cbeb0e07581bf4e8ec133bacb Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 20 Feb 2024 23:42:53 +0100 Subject: [PATCH 10/12] Update lower.cpp --- src/coreclr/jit/lower.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index b78c578a4d5672..a0f326c1de949d 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7903,10 +7903,16 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) else { assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); + const bool isLclVar = data->OperIs(GT_LCL_VAR); // Convert local to LCL_ADDR data->ChangeOper(GT_LCL_ADDR); data->ChangeType(TYP_I_IMPL); + if (isLclVar) + { + // Clear offset if it wasn't a LCL_FLD + data->AsLclFld()->SetLclOffs(0); + } data->ClearContained(); } } From c850c35e2ee8705dfdf5028d3e7b84532341d270 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 20 Feb 2024 23:47:44 +0100 Subject: [PATCH 11/12] Update lower.cpp --- src/coreclr/jit/lower.cpp | 9 +++------ 1 file changed, 3 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index a0f326c1de949d..29f8ac721b3139 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7903,16 +7903,13 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) else { assert(data->OperIs(GT_LCL_VAR, GT_LCL_FLD)); - const bool isLclVar = data->OperIs(GT_LCL_VAR); // Convert local to LCL_ADDR + unsigned lclOffset = data->AsLclVarCommon()->GetLclOffs(); + data->ChangeOper(GT_LCL_ADDR); data->ChangeType(TYP_I_IMPL); - if (isLclVar) - { - // Clear offset if it wasn't a LCL_FLD - data->AsLclFld()->SetLclOffs(0); - } + data->AsLclFld()->SetLclOffs(lclOffset); data->ClearContained(); } } From b3a5350bd2e2e2128d0f8dbb298447fe5ac23160 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 21 Feb 2024 00:39:23 +0100 Subject: [PATCH 12/12] fix failures (don't use blkNode->OperIsInitBlkOp() when we already removed GT_INIT_VAL) --- src/coreclr/jit/lower.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 29f8ac721b3139..ac15eaab00def5 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -7881,9 +7881,13 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) GenTree* data = blkNode->Data(); GenTree* size; + CorInfoHelpFunc helper; + // Is it Memset ... if (blkNode->OperIsInitBlkOp()) { + helper = CORINFO_HELP_MEMSET; + // Drop GT_INIT_VAL nodes if (data->OperIsInitVal()) { @@ -7894,6 +7898,8 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) else { // ... or Memcpy? + helper = CORINFO_HELP_MEMCPY; + if (data->OperIs(GT_IND)) { // Drop GT_IND nodes @@ -7931,7 +7937,6 @@ void Lowering::LowerBlockStoreAsHelperCall(GenTreeBlk* blkNode) GenTree* dataPlaceholder = comp->gtNewZeroConNode(genActualType(data)); GenTree* sizePlaceholder = comp->gtNewZeroConNode(genActualType(size)); - CorInfoHelpFunc helper = blkNode->OperIsInitBlkOp() ? CORINFO_HELP_MEMSET : CORINFO_HELP_MEMCPY; GenTreeCall* call = comp->gtNewHelperCallNode(helper, TYP_VOID, destPlaceholder, dataPlaceholder, sizePlaceholder); comp->fgMorphArgs(call);