diff options
author | Carol Eidt <carol.eidt@microsoft.com> | 2019-06-05 14:51:04 -0700 |
---|---|---|
committer | GitHub <noreply@github.com> | 2019-06-05 14:51:04 -0700 |
commit | 3809a06b68ac70148a19a37cd3cec650ba4f27c7 (patch) | |
tree | 5e42b80a5057e2c26177294d3b1cbbc8f19890ca /src | |
parent | a2db030f1b9f1a5184ea3fd27ccbefb4588d4451 (diff) | |
download | coreclr-3809a06b68ac70148a19a37cd3cec650ba4f27c7.tar.gz coreclr-3809a06b68ac70148a19a37cd3cec650ba4f27c7.tar.bz2 coreclr-3809a06b68ac70148a19a37cd3cec650ba4f27c7.zip |
Cleanup block stores and test for 24846 (#24950)
* Cleanup block stores and test for 24846
Fix zero-length assert/bad codegen for initblk.
Remove redundant assertions in codegen and those that don't directly relate to codegen requirements.
Eliminate redundant LEA that was being generated by `genCodeForCpBlk`.
Rename `genCodeFor[Cp|Init]Blk` to `genCodeFor[Cp|Init]BlkHelper` to parallel the other forms.
Fix the test case for #24846.
Diffstat (limited to 'src')
-rw-r--r-- | src/jit/codegen.h | 8 | ||||
-rw-r--r-- | src/jit/codegenarmarch.cpp | 66 | ||||
-rw-r--r-- | src/jit/codegenlinear.cpp | 9 | ||||
-rw-r--r-- | src/jit/codegenxarch.cpp | 221 | ||||
-rw-r--r-- | src/jit/lowerxarch.cpp | 2 | ||||
-rw-r--r-- | src/jit/morph.cpp | 13 |
6 files changed, 125 insertions, 194 deletions
diff --git a/src/jit/codegen.h b/src/jit/codegen.h index 7919d07857..fcbf4d8ec9 100644 --- a/src/jit/codegen.h +++ b/src/jit/codegen.h @@ -1097,9 +1097,11 @@ protected: void genCodeForStoreInd(GenTreeStoreInd* tree); void genCodeForSwap(GenTreeOp* tree); void genCodeForCpObj(GenTreeObj* cpObjNode); - void genCodeForCpBlk(GenTreeBlk* cpBlkNode); void genCodeForCpBlkRepMovs(GenTreeBlk* cpBlkNode); void genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode); +#ifndef _TARGET_X86_ + void genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode); +#endif void genCodeForPhysReg(GenTreePhysReg* tree); void genCodeForNullCheck(GenTreeOp* tree); void genCodeForCmpXchg(GenTreeCmpXchg* tree); @@ -1173,7 +1175,9 @@ protected: #endif // _TARGET_ARM64_ void genCodeForStoreBlk(GenTreeBlk* storeBlkNode); - void genCodeForInitBlk(GenTreeBlk* initBlkNode); +#ifndef _TARGET_X86_ + void genCodeForInitBlkHelper(GenTreeBlk* initBlkNode); +#endif void genCodeForInitBlkRepStos(GenTreeBlk* initBlkNode); void genCodeForInitBlkUnroll(GenTreeBlk* initBlkNode); void genJumpTable(GenTree* tree); diff --git a/src/jit/codegenarmarch.cpp b/src/jit/codegenarmarch.cpp index c933f45b5c..87cbd6d1d5 100644 --- a/src/jit/codegenarmarch.cpp +++ b/src/jit/codegenarmarch.cpp @@ -1870,26 +1870,22 @@ void CodeGen::genCodeForIndir(GenTreeIndir* tree) genProduceReg(tree); } -// Generate code for a CpBlk node by the means of the VM memcpy helper call +//---------------------------------------------------------------------------------- +// 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: -// a) The size argument of the CpBlk is not an integer constant -// b) The size argument is a constant but is larger than CPBLK_MOVS_LIMIT bytes. -void CodeGen::genCodeForCpBlk(GenTreeBlk* cpBlkNode) +// The register assignments have been set appropriately. +// This is validated by genConsumeBlockOp(). +// +void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode) { - // Make sure we got the arguments of the cpblk operation in the right registers - unsigned blockSize = cpBlkNode->Size(); - GenTree* dstAddr = cpBlkNode->Addr(); - assert(!dstAddr->isContained()); - + // 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); -#ifdef _TARGET_ARM64_ - if (blockSize != 0) - { - assert(blockSize > CPBLK_UNROLL_LIMIT); - } -#endif // _TARGET_ARM64_ - if (cpBlkNode->gtFlags & GTF_BLK_VOLATILE) { // issue a full memory barrier before a volatile CpBlk operation @@ -2057,30 +2053,20 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode) } } -// Generates code for InitBlk by calling the VM memset helper function. +//------------------------------------------------------------------------ +// 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: -// a) The size argument of the InitBlk is not an integer constant. -// b) The size argument of the InitBlk is >= INITBLK_STOS_LIMIT bytes. -void CodeGen::genCodeForInitBlk(GenTreeBlk* initBlkNode) +// The register assignments have been set appropriately. +// This is validated by genConsumeBlockOp(). +// +void CodeGen::genCodeForInitBlkHelper(GenTreeBlk* initBlkNode) { - unsigned size = initBlkNode->Size(); - GenTree* dstAddr = initBlkNode->Addr(); - GenTree* initVal = initBlkNode->Data(); - if (initVal->OperIsInitVal()) - { - initVal = initVal->gtGetOp1(); - } - - assert(!dstAddr->isContained()); - assert(!initVal->isContained()); - -#ifdef _TARGET_ARM64_ - if (size != 0) - { - assert((size > INITBLK_UNROLL_LIMIT) || !initVal->IsCnsIntOrI()); - } -#endif // _TARGET_ARM64_ - + // 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->gtFlags & GTF_BLK_VOLATILE) @@ -3380,11 +3366,11 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp) case GenTreeBlk::BlkOpKindHelper: if (isCopyBlk) { - genCodeForCpBlk(blkOp); + genCodeForCpBlkHelper(blkOp); } else { - genCodeForInitBlk(blkOp); + genCodeForInitBlkHelper(blkOp); } break; diff --git a/src/jit/codegenlinear.cpp b/src/jit/codegenlinear.cpp index dac1df9e56..0ed39a742a 100644 --- a/src/jit/codegenlinear.cpp +++ b/src/jit/codegenlinear.cpp @@ -1690,6 +1690,7 @@ void CodeGen::genConsumeBlockSrc(GenTreeBlk* blkNode) if (blkNode->OperIsCopyBlkOp()) { // For a CopyBlk we need the address of the source. + assert(src->isContained()); if (src->OperGet() == GT_IND) { src = src->gtOp.gtOp1; @@ -1770,9 +1771,15 @@ void CodeGen::genConsumeBlockOp(GenTreeBlk* blkNode, regNumber dstReg, regNumber GenTree* const dstAddr = blkNode->Addr(); - // First, consume all the sources in order. + // First, consume all the sources in order, and verify that registers have been allocated appropriately, + // based on the 'gtBlkOpKind'. + + // The destination is always in a register; 'genConsumeReg' asserts that. genConsumeReg(dstAddr); + // The source may be a local or in a register; 'genConsumeBlockSrc' will check that. genConsumeBlockSrc(blkNode); + // 'genSetBlockSize' (called below) will ensure that a register has been reserved as needed + // in the case where the size is a constant (i.e. it is not GT_STORE_DYN_BLK). if (blkNode->OperGet() == GT_STORE_DYN_BLK) { genConsumeReg(blkNode->AsDynBlk()->gtDynamicSize); diff --git a/src/jit/codegenxarch.cpp b/src/jit/codegenxarch.cpp index 3855e2cfe8..9184c09b39 100644 --- a/src/jit/codegenxarch.cpp +++ b/src/jit/codegenxarch.cpp @@ -2880,11 +2880,11 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* storeBlkNode) case GenTreeBlk::BlkOpKindHelper: if (isCopyBlk) { - genCodeForCpBlk(storeBlkNode); + genCodeForCpBlkHelper(storeBlkNode); } else { - genCodeForInitBlk(storeBlkNode); + genCodeForInitBlkHelper(storeBlkNode); } break; #endif // _TARGET_AMD64_ @@ -2946,29 +2946,16 @@ void CodeGen::genCodeForInitBlkRepStos(GenTreeBlk* initBlkNode) initVal = initVal->gtGetOp1(); } -#ifdef DEBUG - assert(dstAddr->isUsedFromReg()); - assert(initVal->isUsedFromReg()); -#ifdef _TARGET_AMD64_ - assert(size != 0); -#endif - if (initVal->IsCnsIntOrI()) - { -#ifdef _TARGET_AMD64_ - assert(size > CPBLK_UNROLL_LIMIT && size < CPBLK_MOVS_LIMIT); -#else - // Note that a size of zero means a non-constant size. - assert((size == 0) || (size > CPBLK_UNROLL_LIMIT)); -#endif - } - -#endif // DEBUG - genConsumeBlockOp(initBlkNode, REG_RDI, REG_RAX, REG_RCX); instGen(INS_r_stosb); } -// Generate code for InitBlk by performing a loop unroll +//---------------------------------------------------------------------------------- +// genCodeForInitBlkUnroll: Generate code for InitBlk by performing a loop unroll +// +// Arguments: +// initBlkNode - The Block store for which we are generating code. +// // Preconditions: // a) Both the size and fill byte value are integer constants. // b) The size of the struct to initialize is smaller than INITBLK_UNROLL_LIMIT bytes. @@ -2986,7 +2973,6 @@ void CodeGen::genCodeForInitBlkUnroll(GenTreeBlk* initBlkNode) assert(dstAddr->isUsedFromReg()); assert(initVal->isUsedFromReg() || (initVal->IsIntegralConst(0) && ((size & 0xf) == 0))); - assert(size != 0); assert(size <= INITBLK_UNROLL_LIMIT); assert(initVal->gtSkipReloadOrCopy()->IsCnsIntOrI()); @@ -3064,37 +3050,26 @@ void CodeGen::genCodeForInitBlkUnroll(GenTreeBlk* initBlkNode) } } -// Generates code for InitBlk by calling the VM memset helper function. +#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: -// a) The size argument of the InitBlk is not an integer constant. -// b) The size argument of the InitBlk is >= INITBLK_STOS_LIMIT bytes. -void CodeGen::genCodeForInitBlk(GenTreeBlk* initBlkNode) +// The register assignments have been set appropriately. +// This is validated by genConsumeBlockOp(). +// +void CodeGen::genCodeForInitBlkHelper(GenTreeBlk* initBlkNode) { -#ifdef _TARGET_AMD64_ - // Make sure we got the arguments of the initblk operation in the right registers - unsigned blockSize = initBlkNode->Size(); - GenTree* dstAddr = initBlkNode->Addr(); - GenTree* initVal = initBlkNode->Data(); - if (initVal->OperIsInitVal()) - { - initVal = initVal->gtGetOp1(); - } - - assert(dstAddr->isUsedFromReg()); - assert(initVal->isUsedFromReg()); - - if (blockSize != 0) - { - assert(blockSize >= CPBLK_MOVS_LIMIT); - } - + // 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); -#else // !_TARGET_AMD64_ - NYI_X86("Helper call for InitBlk"); -#endif // !_TARGET_AMD64_ } +#endif // _TARGET_AMD64_ // Generate code for a load from some address + offset // baseNode: tree node which can be either a local address or arbitrary node @@ -3146,14 +3121,19 @@ void CodeGen::genCodeForStoreOffset(instruction ins, emitAttr size, regNumber sr } } -// Generates CpBlk code by performing a loop unroll +//---------------------------------------------------------------------------------- +// genCodeForCpBlkUnroll - Generates CpBlk code by performing a loop unroll +// +// Arguments: +// cpBlkNode - the GT_STORE_BLK +// // Preconditions: -// The size argument of the CpBlk node is a constant and <= 64 bytes. -// This may seem small but covers >95% of the cases in several framework assemblies. +// The register assignments have been set appropriately. // void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode) { // Make sure we got the arguments of the cpblk operation in the right registers + assert(cpBlkNode->OperIs(GT_STORE_BLK) && !cpBlkNode->OperIsInitBlkOp()); unsigned size = cpBlkNode->Size(); GenTree* dstAddr = cpBlkNode->Addr(); GenTree* source = cpBlkNode->Data(); @@ -3201,8 +3181,7 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode) if (size >= XMM_REGSIZE_BYTES) { regNumber xmmReg = cpBlkNode->GetSingleTempReg(RBM_ALLFLOAT); - assert(genIsValidFloatReg(xmmReg)); - size_t slots = size / XMM_REGSIZE_BYTES; + size_t slots = size / XMM_REGSIZE_BYTES; // TODO: In the below code the load and store instructions are for 16 bytes, but the // type is EA_8BYTE. The movdqa/u are 16 byte instructions, so it works, but @@ -3258,32 +3237,20 @@ void CodeGen::genCodeForCpBlkUnroll(GenTreeBlk* cpBlkNode) } } -// Generate code for CpBlk by using rep movs +//---------------------------------------------------------------------------------- +// genCodeForCpBlkRepMovs - Generate code for CpBlk by using rep movs +// +// Arguments: +// cpBlkNode - the GT_STORE_[BLK|OBJ|DYN_BLK] +// // Preconditions: -// The size argument of the CpBlk is a constant and is between -// CPBLK_UNROLL_LIMIT and CPBLK_MOVS_LIMIT bytes. +// The register assignments have been set appropriately. +// This is validated by genConsumeBlockOp(). +// void CodeGen::genCodeForCpBlkRepMovs(GenTreeBlk* cpBlkNode) { - // Make sure we got the arguments of the cpblk operation in the right registers - unsigned size = cpBlkNode->Size(); - GenTree* dstAddr = cpBlkNode->Addr(); - GenTree* source = cpBlkNode->Data(); - GenTree* srcAddr = nullptr; - - assert(dstAddr->isUsedFromReg()); - assert(source->isContained()); - -#ifdef _TARGET_X86_ - if (!cpBlkNode->OperIs(GT_STORE_DYN_BLK)) - { - assert((size == 0) || (size > CPBLK_UNROLL_LIMIT)); - } -#else // _TARGET_AMD64_ - // On x64 we use the helper call for GT_STORE_DYN_BLK. - assert(!cpBlkNode->OperIs(GT_STORE_DYN_BLK)); - assert((size == 0) || ((size > CPBLK_UNROLL_LIMIT) && (size < CPBLK_MOVS_LIMIT))); -#endif - + // Destination address goes in RDI, source address goes in RSE, and size goes in RCX. + // genConsumeBlockOp takes care of this for us. genConsumeBlockOp(cpBlkNode, REG_RDI, REG_RSI, REG_RCX); instGen(INS_r_movsb); } @@ -3312,7 +3279,7 @@ unsigned CodeGen::genMove8IfNeeded(unsigned size, regNumber longTmpReg, GenTree* #ifdef _TARGET_X86_ instruction longMovIns = INS_movq; #else // !_TARGET_X86_ - instruction longMovIns = INS_mov; + instruction longMovIns = INS_mov; #endif // !_TARGET_X86_ if ((size & 8) != 0) { @@ -3613,48 +3580,44 @@ void CodeGen::genClearStackVec3ArgUpperBits() #endif // defined(UNIX_AMD64_ABI) && defined(FEATURE_SIMD) #endif // FEATURE_PUT_STRUCT_ARG_STK -// Generate code for CpObj nodes wich copy structs that have interleaved -// GC pointers. -// This will generate a sequence of movsp instructions for the cases of non-gc members. -// Note that movsp is an alias for movsd on x86 and movsq on x64. -// and calls to the BY_REF_ASSIGN helper otherwise. +// +// genCodeForCpObj - Generate code for CpObj nodes to copy structs that have interleaved +// GC pointers. +// +// Arguments: +// cpObjNode - the GT_STORE_OBJ +// +// Notes: +// This will generate a sequence of movsp instructions for the cases of non-gc members. +// Note that movsp is an alias for movsd on x86 and movsq on x64. +// and calls to the BY_REF_ASSIGN helper otherwise. +// +// Preconditions: +// The register assignments have been set appropriately. +// This is validated by genConsumeBlockOp(). +// void CodeGen::genCodeForCpObj(GenTreeObj* cpObjNode) { // Make sure we got the arguments of the cpobj operation in the right registers - GenTree* dstAddr = cpObjNode->Addr(); - GenTree* source = cpObjNode->Data(); - GenTree* srcAddr = nullptr; - var_types srcAddrType = TYP_BYREF; - bool sourceIsLocal = false; - - assert(source->isContained()); - if (source->gtOper == GT_IND) - { - srcAddr = source->gtGetOp1(); - assert(srcAddr->isUsedFromReg()); - } - else - { - noway_assert(source->IsLocal()); - sourceIsLocal = true; - } - - bool dstOnStack = dstAddr->gtSkipReloadOrCopy()->OperIsLocalAddr(); + GenTree* dstAddr = cpObjNode->Addr(); + GenTree* source = cpObjNode->Data(); + GenTree* srcAddr = nullptr; + var_types srcAddrType = TYP_BYREF; + bool dstOnStack = dstAddr->gtSkipReloadOrCopy()->OperIsLocalAddr(); #ifdef DEBUG - assert(dstAddr->isUsedFromReg()); - - // If the GenTree node has data about GC pointers, this means we're dealing - // with CpObj, so this requires special logic. + // A CpObj always involves one or more GC pointers. assert(cpObjNode->gtGcPtrCount > 0); // MovSp (alias for movsq on x64 and movsd on x86) instruction is used for copying non-gcref fields // and it needs src = RSI and dst = RDI. // Either these registers must not contain lclVars, or they must be dying or marked for spill. // This is because these registers are incremented as we go through the struct. - if (!sourceIsLocal) + if (!source->IsLocal()) { + assert(source->gtOper == GT_IND); + srcAddr = source->gtGetOp1(); GenTree* actualSrcAddr = srcAddr->gtSkipReloadOrCopy(); GenTree* actualDstAddr = dstAddr->gtSkipReloadOrCopy(); unsigned srcLclVarNum = BAD_VAR_NUM; @@ -3771,46 +3734,26 @@ void CodeGen::genCodeForCpObj(GenTreeObj* cpObjNode) gcInfo.gcMarkRegSetNpt(RBM_RDI); } -// Generate code for a CpBlk node by the means of the VM memcpy helper call +#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: -// a) The size argument of the CpBlk is not an integer constant -// b) The size argument is a constant but is larger than CPBLK_MOVS_LIMIT bytes. -void CodeGen::genCodeForCpBlk(GenTreeBlk* cpBlkNode) +// The register assignments have been set appropriately. +// This is validated by genConsumeBlockOp(). +// +void CodeGen::genCodeForCpBlkHelper(GenTreeBlk* cpBlkNode) { -#ifdef _TARGET_AMD64_ - // Make sure we got the arguments of the cpblk operation in the right registers - unsigned blockSize = cpBlkNode->Size(); - GenTree* dstAddr = cpBlkNode->Addr(); - GenTree* source = cpBlkNode->Data(); - GenTree* srcAddr = nullptr; - - // Size goes in arg2 - if (cpBlkNode->gtOper != GT_STORE_DYN_BLK) - { - assert(blockSize >= CPBLK_MOVS_LIMIT); - assert((cpBlkNode->gtRsvdRegs & RBM_ARG_2) != 0); - } - - // Source address goes in arg1 - if (source->gtOper == GT_IND) - { - srcAddr = source->gtGetOp1(); - assert(srcAddr->isUsedFromReg()); - } - else - { - noway_assert(source->IsLocal()); - assert((cpBlkNode->gtRsvdRegs & RBM_ARG_1) != 0); - inst_RV_TT(INS_lea, REG_ARG_1, source, 0, EA_BYREF); - } - + // 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); -#else // !_TARGET_AMD64_ - noway_assert(false && "Helper call for CpBlk is not needed."); -#endif // !_TARGET_AMD64_ } +#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/jit/lowerxarch.cpp b/src/jit/lowerxarch.cpp index d29ea9375f..613f6cb9f8 100644 --- a/src/jit/lowerxarch.cpp +++ b/src/jit/lowerxarch.cpp @@ -188,7 +188,7 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode) unsigned helperThreshold = max(INITBLK_STOS_LIMIT, INITBLK_UNROLL_LIMIT); // TODO-X86-CQ: Investigate whether a helper call would be beneficial on x86 - if (size != 0 && size <= helperThreshold) + if (!blkNode->OperIs(GT_STORE_DYN_BLK) && size <= helperThreshold) { // Always favor unrolling vs rep stos. if (size <= INITBLK_UNROLL_LIMIT && initVal->IsCnsIntOrI()) diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp index 3bfca379a8..5c13c2bc1a 100644 --- a/src/jit/morph.cpp +++ b/src/jit/morph.cpp @@ -9478,16 +9478,7 @@ GenTree* Compiler::fgMorphInitBlock(GenTree* tree) } else { - if (dest->OperIs(GT_DYN_BLK)) - { - blockSize = 0; - } - else - { - assert(dest->OperIs(GT_BLK, GT_OBJ)); - blockSize = dest->AsBlk()->Size(); - assert(blockSize != 0); - } + blockSize = dest->AsBlk()->Size(); FieldSeqNode* destFldSeq = nullptr; if (dest->AsIndir()->Addr()->IsLocalAddrExpr(this, &destLclNode, &destFldSeq)) @@ -9604,7 +9595,7 @@ GenTree* Compiler::fgMorphPromoteLocalInitBlock(GenTreeLclVar* destLclNode, GenT if (blockSize == 0) { - JITDUMP(" size is not known.\n"); + JITDUMP(" size is zero or unknown.\n"); return nullptr; } |