From a7a3706e77746aef7ab5866c53bfad13a84c94ad Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 23 Apr 2020 22:49:52 -0700 Subject: [PATCH 01/30] Mark Div/Mod nodes that will use reciprocal multiply with GTF_DIV_USE_MAGIC --- src/coreclr/src/jit/gentree.cpp | 139 ++++++++++++++++++++++++++++++++ src/coreclr/src/jit/gentree.h | 15 ++++ src/coreclr/src/jit/gtlist.h | 2 +- src/coreclr/src/jit/lower.cpp | 4 +- src/coreclr/src/jit/morph.cpp | 8 ++ 5 files changed, 166 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 25d4bc58054858..2333d2157644ae 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -6674,6 +6674,133 @@ void GenTreeIntCon::FixupInitBlkValue(var_types asgType) } } +//---------------------------------------------------------------------------- +// usesMagicNumberDivision: returns true if rationalize will use MagicNumber +// multiplication for this node. +// +// Arguments: +// this - A GenTreeOp binary or unary node +// +// Return Value: +// Return true iff the node is a GT_DIV,GT_UDIV, GT_MOD or GT_UMOD with +// an integer constant and we can perform the division operation using +// a reciprocal multiply or a shift operation. +// +bool GenTreeOp::usesMagicNumberDivision(Compiler* comp) +{ + if (!OperIs(GT_DIV, GT_MOD, GT_UDIV, GT_UMOD)) + { + return false; + } +#if defined(TARGET_ARM64) + if (OperIs(GT_MOD, GT_UMOD)) + { + // MOD, UMOD not supported for ARM64 + return false; + } +#endif // TARGET_ARM64 + + bool isSignedDivide = OperIs(GT_DIV, GT_MOD); + GenTree* dividend = gtGetOp1(); + GenTree* divisor = gtGetOp2(); + +#if !defined(TARGET_64BIT) + if (dividend->OperIs(GT_LONG)) + { + return false; + } +#endif + + if (!divisor->IsCnsIntOrI()) + { + return false; + } + + if (dividend->IsCnsIntOrI()) + { + // We shouldn't see a divmod with constant operands here but if we do then it's likely + // because optimizations are disabled or it's a case that's supposed to throw an exception. + // Don't optimize this. + return false; + } + + const var_types divType = TypeGet(); + assert((divType == TYP_INT) || (divType == TYP_I_IMPL)); + + ssize_t divisorValue = static_cast(divisor->AsIntCon()->IconValue()); + + if (divType == TYP_INT) + { + // Clear up the upper 32 bits of the value, they may be set to 1 because constants + // are treated as signed and stored in ssize_t which is 64 bit in size on 64 bit targets. + divisorValue &= UINT32_MAX; + } + + if (divisorValue == 0) + { + // x / 0 and x % 0 can't be optimized because they are required to throw an exception. + return false; + } + else if (isSignedDivide && (divisorValue == -1)) + { + // x / -1 can't be optimized because INT_MIN / -1 is required to throw an exception. + return false; + } + else if (isPow2(divisorValue)) + { + return true; + } + + const bool isDiv = OperIs(GT_DIV, GT_UDIV); + + if (isDiv) + { + if (isSignedDivide) + { + // If the divisor is the minimum representable integer value then the result is either 0 or 1 + if ((divType == TYP_INT && divisorValue == INT_MIN) || (divType == TYP_LONG && divisorValue == INT64_MIN)) + { + return true; + } + } + else + { + // If the divisor is greater or equal than 2^(N - 1) then the result is either 0 or 1 + if (((divType == TYP_INT) && (divisorValue > (UINT32_MAX / 2))) || + ((divType == TYP_LONG) && (divisorValue > (UINT64_MAX / 2)))) + { + return true; + } + } + } + +// TODO-ARM-CQ: Currently there's no GT_MULHI for ARM32 +#if defined(TARGET_XARCH) || defined(TARGET_ARM64) + if (!comp->opts.MinOpts() && ((divisorValue >= 3) || !isSignedDivide)) + { + // All checks pass we can perform the division operation using a reciprocal multiply. + return true; + } +#endif + + return false; +} + +// checks if rationalize will use MagicNumber multiplication for this node +// and sets the flag GTF_DIV_USE_MAGIC +void GenTreeOp::checkMagicNumberDivision(Compiler* comp) +{ + if (usesMagicNumberDivision(comp)) + { + gtFlags |= GTF_DIV_USE_MAGIC; + + // Now set DONT_CSE on the GT_CNS_INT divisor + GenTree* divisor = gtGetOp2(); + assert(divisor->IsCnsIntOrI()); + divisor->gtFlags |= GTF_DONT_CSE; + } +} + // //------------------------------------------------------------------------ // gtBlockOpInit: Initializes a BlkOp GenTree @@ -9899,6 +10026,18 @@ void Compiler::gtDispNode(GenTree* tree, IndentStack* indentStack, __in __in_z _ } goto DASH; + case GT_DIV: + case GT_MOD: + case GT_UDIV: + case GT_UMOD: + if (tree->gtFlags & GTF_DIV_USE_MAGIC) + { + printf("M"); + --msgLength; + break; + } + goto DASH; + case GT_LCL_FLD: case GT_LCL_VAR: case GT_LCL_VAR_ADDR: diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 835b7e9089ad8f..2e66f42c02853b 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -922,6 +922,8 @@ struct GenTree #define GTF_OVERFLOW 0x10000000 // Supported for: GT_ADD, GT_SUB, GT_MUL and GT_CAST. // Requires an overflow check. Use gtOverflow(Ex)() to check this flag. +#define GTF_DIV_USE_MAGIC 0x80000000 // GT_DIV -- Uses MagicNumber multiplication to compute this division by a constant + #define GTF_ARR_BOUND_INBND 0x80000000 // GT_ARR_BOUNDS_CHECK -- have proved this check is always in-bounds #define GTF_ARRLEN_ARR_IDX 0x80000000 // GT_ARR_LENGTH -- Length which feeds into an array index expression @@ -2853,6 +2855,19 @@ struct GenTreeOp : public GenTreeUnOp assert(oper == GT_NOP || oper == GT_RETURN || oper == GT_RETFILT || OperIsBlk(oper)); } + // returns true if we will use MagicNumber multiplication for this node. + bool usesMagicNumberDivision(Compiler* comp); + + // checks if we will use MagicNumber multiplication for this node + // then sets the flag GTF_DIV_USE_MAGIC and GTF_DONT_CSE on the constant + void checkMagicNumberDivision(Compiler* comp); + + // True if this node is marked as using MagicNumberDivision + bool markedMagicNumberDivision() const + { + return (gtFlags & GTF_DIV_USE_MAGIC) != 0; + } + #if DEBUGGABLE_GENTREE GenTreeOp() : GenTreeUnOp(), gtOp2(nullptr) { diff --git a/src/coreclr/src/jit/gtlist.h b/src/coreclr/src/jit/gtlist.h index 640affba218b36..bbd44cf7112326 100644 --- a/src/coreclr/src/jit/gtlist.h +++ b/src/coreclr/src/jit/gtlist.h @@ -131,7 +131,7 @@ GTNODE(RSZ , GenTreeOp ,0,GTK_BINOP) GTNODE(ROL , GenTreeOp ,0,GTK_BINOP) GTNODE(ROR , GenTreeOp ,0,GTK_BINOP) GTNODE(MULHI , GenTreeOp ,1,GTK_BINOP) // returns high bits (top N bits of the 2N bit result of an NxN multiply) - // GT_MULHI is used in division by a constant (fgMorphDivByConst). We turn + // GT_MULHI is used in division by a constant (see MagicDivide). We turn // the div into a MULHI + some adjustments. In codegen, we only use the // results of the high register, and we drop the low results. diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index 3ee8b05a89f1e0..ef0c27128f52d7 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -5098,6 +5098,7 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod) divMod->SetOper(newOper); divisor->AsIntCon()->SetIconValue(divisorValue); ContainCheckNode(divMod); + assert(divMod->markedMagicNumberDivision()); return true; } if (isDiv) @@ -5110,6 +5111,7 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod) divMod->SetOper(GT_GE); divMod->gtFlags |= GTF_UNSIGNED; ContainCheckNode(divMod); + assert(divMod->markedMagicNumberDivision()); return true; } } @@ -5134,6 +5136,7 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod) unreached(); #endif } + assert(divMod->markedMagicNumberDivision()); // Depending on the "add" flag returned by GetUnsignedMagicNumberForDivide we need to generate: // add == false (when divisor == 3 for example): @@ -5207,7 +5210,6 @@ bool Lowering::LowerUnsignedDivOrMod(GenTreeOp* divMod) BlockRange().InsertBefore(divMod, div, divisor, mul, dividend); } ContainCheckRange(firstNode, divMod); - return true; } #endif diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 61ed33e644f27f..d0e5fa272ba18d 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -11829,6 +11829,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) { tree = gtFoldExpr(tree); } + + tree->AsOp()->checkMagicNumberDivision(this); return tree; } } @@ -14537,7 +14539,11 @@ GenTree* Compiler::fgMorphSmpOpOptional(GenTreeOp* tree) DEBUG_DESTROY_NODE(tree); return op1; } + break; + case GT_UDIV: + case GT_UMOD: + tree->checkMagicNumberDivision(this); break; case GT_LSH: @@ -14690,6 +14696,8 @@ GenTree* Compiler::fgMorphModToSubMulDiv(GenTreeOp* tree) sub->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED; #endif + tree->checkMagicNumberDivision(this); + return sub; } From 0c8925079d806e891ecea7a494c06404a581e7fb Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 14 May 2020 12:45:08 -0700 Subject: [PATCH 02/30] Update the LclVar ref counts for new CSE LclVars --- src/coreclr/src/jit/optcse.cpp | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 46e834df0dc4f0..fc772e3bbe45e2 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -2678,12 +2678,12 @@ class CSE_Heuristic cseSsaNum = m_pCompiler->lvaTable[cseLclVarNum].lvPerSsaData.AllocSsaNum(allocator); } -#ifdef DEBUG // Verify that all of the ValueNumbers in this list are correct as // Morph will change them when it performs a mutating operation. // ValueNum firstVN = ValueNumStore::NoVN; ValueNum currVN; + bool setRefCnt = true; bool allSame = true; lst = dsc->csdTreeList; @@ -2703,11 +2703,34 @@ class CSE_Heuristic else if (currVN != firstVN) { allSame = false; - break; + } + + BasicBlock* blk = lst->tslBlock; + const BasicBlock::weight_t weight = blk->getBBWeight(m_pCompiler); + + if (setRefCnt) + { + m_pCompiler->lvaTable[cseLclVarNum].setLvRefCnt(1); + m_pCompiler->lvaTable[cseLclVarNum].setLvRefCntWtd(weight); + setRefCnt = false; + } + else + { + m_pCompiler->lvaTable[cseLclVarNum].incRefCnts(weight, m_pCompiler); + } + + // A CSE Def references the LclVar twice + // + GenTree* exp = lst->tslTree; + if (IS_CSE_DEF(exp->gtCSEnum)) + { + m_pCompiler->lvaTable[cseLclVarNum].incRefCnts(weight, m_pCompiler); } } lst = lst->tslNext; } + +#ifdef DEBUG if (!allSame) { lst = dsc->csdTreeList; From e1716fa4b1ee5226fe75937c3c2c198cae9e6c4b Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 14 May 2020 13:23:42 -0700 Subject: [PATCH 03/30] Change csdHashKey to INT64, change the CSE hash key to size_t --- src/coreclr/src/jit/compiler.h | 2 +- src/coreclr/src/jit/optcse.cpp | 19 +++++++++++-------- 2 files changed, 12 insertions(+), 9 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 6e5f954825113d..32ce7ebed5b7c3 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6320,7 +6320,7 @@ class Compiler { CSEdsc* csdNextInBucket; // used by the hash table - unsigned csdHashKey; // the orginal hashkey + INT64 csdHashKey; // the orginal hashkey unsigned csdIndex; // 1..optCSECandidateCount bool csdLiveAcrossCall; diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index fc772e3bbe45e2..1fc4cf39fc083d 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -401,7 +401,7 @@ void Compiler::optValnumCSE_Init() // unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { - unsigned key; + size_t key; unsigned hash; unsigned hval; CSEdsc* hashDsc; @@ -446,11 +446,11 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // if (vnOp2Lib != vnLib) { - key = (unsigned)vnLib; // include the exc set in the hash key + key = (size_t)vnLib; // include the exc set in the hash key } else { - key = (unsigned)vnLibNorm; + key = (size_t)vnLibNorm; } // If we didn't do the above we would have op1 as the CSE def @@ -461,12 +461,15 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) } else // Not a GT_COMMA { - key = (unsigned)vnLibNorm; + key = (size_t)vnLibNorm; } // Compute the hash value for the expression - hash = key; + hash = (unsigned)key; +#ifdef TARGET_64BIT + hash ^= (unsigned)(key >> 32); +#endif hash *= (unsigned)(s_optCSEhashSize + 1); hash >>= 7; @@ -478,7 +481,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) for (hashDsc = optCSEhash[hval]; hashDsc; hashDsc = hashDsc->csdNextInBucket) { - if (hashDsc->csdHashKey == key) + if (hashDsc->csdHashKey == (INT64) key) { treeStmtLst* newElem; @@ -584,7 +587,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { hashDsc = new (this, CMK_CSE) CSEdsc; - hashDsc->csdHashKey = key; + hashDsc->csdHashKey = (INT64) key; hashDsc->csdIndex = 0; hashDsc->csdLiveAcrossCall = false; hashDsc->csdDefCount = 0; @@ -646,7 +649,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) if (verbose) { printf("\nCSE candidate #%02u, vn=", CSEindex); - vnPrint(key, 0); + vnPrint((ValueNum) key, 0); printf(" in " FMT_BB ", [cost=%2u, size=%2u]: \n", compCurBB->bbNum, tree->GetCostEx(), tree->GetCostSz()); gtDispTree(tree); } From fc4cddb19dcee24fa9a56f75ad64b09c1494606d Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 14 May 2020 13:43:47 -0700 Subject: [PATCH 04/30] Update usesMagicNumberDivision() to handle some additional cases --- src/coreclr/src/jit/gentree.cpp | 30 +++++++++++++++++++----------- 1 file changed, 19 insertions(+), 11 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 2333d2157644ae..1ac9260afbbb76 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -6711,11 +6711,6 @@ bool GenTreeOp::usesMagicNumberDivision(Compiler* comp) } #endif - if (!divisor->IsCnsIntOrI()) - { - return false; - } - if (dividend->IsCnsIntOrI()) { // We shouldn't see a divmod with constant operands here but if we do then it's likely @@ -6724,12 +6719,26 @@ bool GenTreeOp::usesMagicNumberDivision(Compiler* comp) return false; } - const var_types divType = TypeGet(); - assert((divType == TYP_INT) || (divType == TYP_I_IMPL)); - - ssize_t divisorValue = static_cast(divisor->AsIntCon()->IconValue()); + ssize_t divisorValue; + if (divisor->IsCnsIntOrI()) + { + divisorValue = static_cast(divisor->AsIntCon()->IconValue()); + } + else + { + ValueNum vn = divisor->gtVNPair.GetLiberal(); + if (comp->vnStore->IsVNConstant(vn)) + { + divisorValue = comp->vnStore->CoercedConstantValue(vn); + } + else + { + return false; + } + } - if (divType == TYP_INT) + const var_types divType = TypeGet(); + if ((divType == TYP_INT) && !isSignedDivide) { // Clear up the upper 32 bits of the value, they may be set to 1 because constants // are treated as signed and stored in ssize_t which is 64 bit in size on 64 bit targets. @@ -6796,7 +6805,6 @@ void GenTreeOp::checkMagicNumberDivision(Compiler* comp) // Now set DONT_CSE on the GT_CNS_INT divisor GenTree* divisor = gtGetOp2(); - assert(divisor->IsCnsIntOrI()); divisor->gtFlags |= GTF_DONT_CSE; } } From 333ceef4ed104f76f9d5efccbfe6abae4dd3b179 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 14 May 2020 15:03:05 -0700 Subject: [PATCH 05/30] Update the gtCost values for constant nodes: GT_CNS_INT, GT_CNS_LNG, GT_CNS_STR ARM64 -11528 : System.Private.CoreLib.dasm (-0.10% of base) 252 total methods with Code Size differences (222 improved, 30 regressed), 20696 unchanged. X64 886 : System.Private.CoreLib.dasm (0.03% of base) 229 total methods with Code Size differences (16 improved, 213 regressed), 20918 unchanged. --- src/coreclr/src/jit/gentree.cpp | 106 +++++++++++++++++++++++++++----- 1 file changed, 91 insertions(+), 15 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 1ac9260afbbb76..1a7ef7a5979f27 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3297,35 +3297,54 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) #elif defined TARGET_XARCH - case GT_CNS_LNG: - costSz = 10; - costEx = 3; - goto COMMON_CNS; - case GT_CNS_STR: +#ifdef TARGET_AMD64 + costSz = 10; + costEx = 1; +#else // TARGET_X86 costSz = 4; costEx = 1; +#endif goto COMMON_CNS; + case GT_CNS_LNG: case GT_CNS_INT: { + ssize_t conVal; + bool fitsInVal = true; + + GenTreeIntConCommon* con = tree->AsIntConCommon(); +#ifndef TARGET_64BIT + if (oper == GT_CNS_LNG) + { + INT64 lngVal = con->LngValue(); + + conVal = (ssize_t)lngVal; // truncate to 32-bits + + fitsInVal = ((INT64)conVal == lngVal); + } + else + { + conVal = con->IconValue(); + } +#else + conVal = con->IconValue(); +#endif // If the constant is a handle then it will need to have a relocation // applied to it. // - GenTreeIntConCommon* con = tree->AsIntConCommon(); - bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - if (!iconNeedsReloc && con->FitsInI8()) + if (!iconNeedsReloc && fitsInVal && GenTreeIntConCommon::FitsInI8(conVal)) { costSz = 1; costEx = 1; } #if defined(TARGET_AMD64) - else if (iconNeedsReloc || !con->FitsInI32()) + else if (iconNeedsReloc || !GenTreeIntConCommon::FitsInI32(conVal)) { costSz = 10; - costEx = 3; + costEx = 1; } #endif // TARGET_AMD64 else @@ -3333,21 +3352,78 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) costSz = 4; costEx = 1; } +#if defined(TARGET_X86) + if (oper == GT_CNS_LNG) + { + costSz += fitsInVal ? 1 : 4; + costEx += 1; + } +#endif goto COMMON_CNS; } #elif defined(TARGET_ARM64) - case GT_CNS_LNG: case GT_CNS_STR: - case GT_CNS_INT: - // TODO-ARM64-NYI: Need cost estimates. - costSz = 1; + costSz = 10; costEx = 1; goto COMMON_CNS; -#else case GT_CNS_LNG: + case GT_CNS_INT: + { + GenTreeIntConCommon* con = tree->AsIntConCommon(); + bool iconNeedsReloc = con->ImmedValNeedsReloc(this); + INT64 imm = con->LngValue(); + emitAttr size = EA_8BYTE; + + if ((imm >= -256) && (imm < 1024)) + { + costSz = 2; + costEx = 1; + } + else if (emitter::emitIns_valid_imm_for_mov(imm, size)) + { + costSz = 4; + costEx = 1; + } + else + { + // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword + // There are three forms + // movk which loads into any halfword preserving the remaining halfwords + // movz which loads into any halfword zeroing the remaining halfwords + // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting the register + // In some cases it is preferable to use movn, because it has the side effect of filling the other halfwords + // with ones + + // Determine whether movn or movz will require the fewest instructions to populate the immediate + bool preferMovz = false; + bool preferMovn = false; + int instructionCount = 4; + + for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) + { + if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) + { + preferMovz = true; // by using a movk to start we can save one instruction + instructionCount--; + } + else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) + { + preferMovn = true; // by using a movn to start we can save one instruction + instructionCount--; + } + } + + costSz = 4 * instructionCount; + costEx = instructionCount; + } + goto COMMON_CNS; + } + +#else case GT_CNS_STR: + case GT_CNS_LNG: case GT_CNS_INT: #error "Unknown TARGET" #endif From 33fb8b0a35665f2b862f0602b94c32b38da363cf Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Sat, 16 May 2020 15:36:40 -0700 Subject: [PATCH 06/30] Fix usesMagicNumberDivision for unsigned divide of MIN_INT Fixes 4 x86 test failures --- src/coreclr/src/jit/gentree.cpp | 35 ++++++++++++++++++++++----------- 1 file changed, 24 insertions(+), 11 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 1a7ef7a5979f27..fee3ff897ae665 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -6814,28 +6814,41 @@ bool GenTreeOp::usesMagicNumberDivision(Compiler* comp) } const var_types divType = TypeGet(); - if ((divType == TYP_INT) && !isSignedDivide) - { - // Clear up the upper 32 bits of the value, they may be set to 1 because constants - // are treated as signed and stored in ssize_t which is 64 bit in size on 64 bit targets. - divisorValue &= UINT32_MAX; - } if (divisorValue == 0) { // x / 0 and x % 0 can't be optimized because they are required to throw an exception. return false; } - else if (isSignedDivide && (divisorValue == -1)) + else if (isSignedDivide) { - // x / -1 can't be optimized because INT_MIN / -1 is required to throw an exception. - return false; + if (divisorValue == -1) + { + // x / -1 can't be optimized because INT_MIN / -1 is required to throw an exception. + return false; + } + else if (isPow2(divisorValue)) + { + return true; + } } - else if (isPow2(divisorValue)) + else // unsigned divide { - return true; + if (divType == TYP_INT) + { + // Clear up the upper 32 bits of the value, they may be set to 1 because constants + // are treated as signed and stored in ssize_t which is 64 bit in size on 64 bit targets. + divisorValue &= UINT32_MAX; + } + + size_t unsignedDivisorValue = (size_t)divisorValue; + if (isPow2(unsignedDivisorValue)) + { + return true; + } } + const bool isDiv = OperIs(GT_DIV, GT_UDIV); if (isDiv) From 5d017f8e826b25bcc7eeeb306d519c54502baa1e Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 15:55:18 -0700 Subject: [PATCH 07/30] If a tree node has had SetRegNum() called to assign it to a physical register and that node becomes a CSE then we clear that info on the CSE expression and instead set it on the CSE LclVar. --- src/coreclr/src/jit/gentree.cpp | 4 ---- src/coreclr/src/jit/gentree.h | 6 ++++++ src/coreclr/src/jit/optcse.cpp | 3 +++ 3 files changed, 9 insertions(+), 4 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index fee3ff897ae665..26df8718651a7f 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -615,10 +615,6 @@ void GenTree::CopyReg(GenTree* from) // GT_COPY/GT_RELOAD is considered having a reg if it // has a reg assigned to any of its positions. // -// Assumption: -// In order for this to work properly, gtClearReg must be called -// prior to setting the register value. -// bool GenTree::gtHasReg() const { bool hasReg = false; diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 2e66f42c02853b..0f7a1e98001b29 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -633,6 +633,12 @@ struct GenTree assert(_gtRegNum == reg); } + void ClearRegNum() + { + _gtRegNum = REG_NA; + INDEBUG(gtRegTag = GT_REGTAG_NONE;) + } + // Copy the _gtRegNum/gtRegTag fields void CopyReg(GenTree* from); bool gtHasReg() const; diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 1fc4cf39fc083d..ef31aef73a0d39 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -3016,6 +3016,9 @@ class CSE_Heuristic // cannot add any new exceptions } + cse->CopyReg(exp); // The cse inheirits any reg num property from the orginal exp node + exp->ClearRegNum(); // The exp node (for a CSE def) no longer has a register requirement + // Walk the statement 'stmt' and find the pointer // in the tree is pointing to 'exp' // From 9d871485bfd5f604d951595964b5969f03299aa4 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 16:13:01 -0700 Subject: [PATCH 08/30] As part of the CSE of constants work, once we make a CSE we don't want constant prop to undo the CSE operation We re-purpose the GTF_DONT_CSE flag to tell assertionprop not to perform constant prop on this CSE value --- src/coreclr/src/jit/assertionprop.cpp | 6 ++++++ src/coreclr/src/jit/optcse.cpp | 2 ++ 2 files changed, 8 insertions(+) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index 987388a39259dc..c50c46a115e5c8 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -4947,6 +4947,12 @@ GenTree* Compiler::optVNConstantPropOnJTrue(BasicBlock* block, GenTree* test) // Compiler::fgWalkResult Compiler::optVNConstantPropCurStmt(BasicBlock* block, Statement* stmt, GenTree* tree) { + // Don't perform const prop on expressions marked with GTF_DONT_CSE + if (!tree->CanCSE()) + { + return WALK_CONTINUE; + } + // Don't propagate floating-point constants into a TYP_STRUCT LclVar // This can occur for HFA return values (see hfa_sf3E_r.exe) if (tree->TypeGet() == TYP_STRUCT) diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index ef31aef73a0d39..4b90b9a5adca83 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -2818,6 +2818,7 @@ class CSE_Heuristic // ValueNumStore* vnStore = m_pCompiler->vnStore; cse = m_pCompiler->gtNewLclvNode(cseLclVarNum, cseLclVarTyp); + cse->SetDoNotCSE(); // Assign the ssa num for the use. Note it may be the reserved num. cse->AsLclVarCommon()->SetSsaNum(cseSsaNum); @@ -3005,6 +3006,7 @@ class CSE_Heuristic /* Create a reference to the CSE temp */ GenTree* ref = m_pCompiler->gtNewLclvNode(cseLclVarNum, cseLclVarTyp); ref->gtVNPair = val->gtVNPair; // The new 'ref' is the same as 'val' + ref->SetDoNotCSE(); // Assign the ssa num for the ref use. Note it may be the reserved num. ref->AsLclVarCommon()->SetSsaNum(cseSsaNum); From f42725638aaf4768039b9bf1dff65f7f48bef49f Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 16:52:18 -0700 Subject: [PATCH 09/30] When we duplicate a conditional test to change a loop into a bottom tested loop We also will record that the block has a call if the duplicated code contains a call. --- src/coreclr/src/jit/block.cpp | 4 ++++ src/coreclr/src/jit/optimizer.cpp | 5 +++++ 2 files changed, 9 insertions(+) diff --git a/src/coreclr/src/jit/block.cpp b/src/coreclr/src/jit/block.cpp index 9064caaaa38e4b..d8ca31c6662c24 100644 --- a/src/coreclr/src/jit/block.cpp +++ b/src/coreclr/src/jit/block.cpp @@ -309,6 +309,10 @@ void BasicBlock::dspFlags() { printf("jmp "); } + if (bbFlags & BBF_HAS_CALL) + { + printf("hascall "); + } if (bbFlags & BBF_GC_SAFE_POINT) { printf("gcsafe "); diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp index adc874b27887c5..17a05bc1ce06e1 100644 --- a/src/coreclr/src/jit/optimizer.cpp +++ b/src/coreclr/src/jit/optimizer.cpp @@ -4265,6 +4265,11 @@ void Compiler::fgOptWhileLoop(BasicBlock* block) copyOfCondStmt->SetCompilerAdded(); + if (condTree->gtFlags & GTF_CALL) + { + block->bbFlags |= BBF_HAS_CALL; // Record that the block has a call + } + if (opts.compDbgInfo) { copyOfCondStmt->SetILOffsetX(condStmt->GetILOffsetX()); From 140d6dd462a3e859d945b6c188548b7d60370fc8 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 17:26:32 -0700 Subject: [PATCH 10/30] In invariant hoisting the hoist Expr does not have to computed into a specific register, so clear the RegNum if it was set in the original expression. --- src/coreclr/src/jit/optimizer.cpp | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/optimizer.cpp b/src/coreclr/src/jit/optimizer.cpp index 17a05bc1ce06e1..ac56ff3ed4afb3 100644 --- a/src/coreclr/src/jit/optimizer.cpp +++ b/src/coreclr/src/jit/optimizer.cpp @@ -4269,7 +4269,7 @@ void Compiler::fgOptWhileLoop(BasicBlock* block) { block->bbFlags |= BBF_HAS_CALL; // Record that the block has a call } - + if (opts.compDbgInfo) { copyOfCondStmt->SetILOffsetX(condStmt->GetILOffsetX()); @@ -6203,6 +6203,10 @@ void Compiler::optPerformHoistExpr(GenTree* origExpr, unsigned lnum) // Create a copy of the expression and mark it for CSE's. GenTree* hoistExpr = gtCloneExpr(origExpr, GTF_MAKE_CSE); + // The hoist Expr does not have to computed into a specific register, + // so clear the RegNum if it was set in the original expression + hoistExpr->ClearRegNum(); + // At this point we should have a cloned expression, marked with the GTF_MAKE_CSE flag assert(hoistExpr != origExpr); assert(hoistExpr->gtFlags & GTF_MAKE_CSE); From 29d02eafc278b541483e9bbb63b7e5c225b8880f Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 20:59:58 -0700 Subject: [PATCH 11/30] Change the weights of GT_CNS_INT for ARM64 Add support for COMPLUS_JitDisableConstCSE --- src/coreclr/src/jit/gentree.cpp | 173 +++++++++++++++++--------- src/coreclr/src/jit/jitconfigvalues.h | 4 + 2 files changed, 120 insertions(+), 57 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 26df8718651a7f..d603221517639c 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3306,11 +3306,11 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_CNS_LNG: case GT_CNS_INT: { - ssize_t conVal; + GenTreeIntConCommon* con = tree->AsIntConCommon(); + ssize_t conVal = (oper == GT_CNS_LNG) ? (ssize_t)con->LngValue() : con->IconValue(); bool fitsInVal = true; - GenTreeIntConCommon* con = tree->AsIntConCommon(); -#ifndef TARGET_64BIT +#ifdef TARGET_X86 if (oper == GT_CNS_LNG) { INT64 lngVal = con->LngValue(); @@ -3319,25 +3319,25 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) fitsInVal = ((INT64)conVal == lngVal); } - else - { - conVal = con->IconValue(); - } -#else - conVal = con->IconValue(); -#endif +#endif // TARGET_X86 + // If the constant is a handle then it will need to have a relocation // applied to it. // bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - if (!iconNeedsReloc && fitsInVal && GenTreeIntConCommon::FitsInI8(conVal)) + if (iconNeedsReloc) + { + costSz = 4; + costEx = 1; + } + else if (fitsInVal && GenTreeIntConCommon::FitsInI8(conVal)) { costSz = 1; costEx = 1; } -#if defined(TARGET_AMD64) - else if (iconNeedsReloc || !GenTreeIntConCommon::FitsInI32(conVal)) +#ifdef TARGET_AMD64 + else if (!GenTreeIntConCommon::FitsInI32(conVal)) { costSz = 10; costEx = 1; @@ -3348,74 +3348,133 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) costSz = 4; costEx = 1; } -#if defined(TARGET_X86) +#ifdef TARGET_X86 if (oper == GT_CNS_LNG) { costSz += fitsInVal ? 1 : 4; costEx += 1; } -#endif +#endif // TARGET_X86 + + if (JitConfig.JitDisableConstCSE() == 3) + { + GenTreeIntConCommon* con = tree->AsIntConCommon(); + bool iconNeedsReloc = con->ImmedValNeedsReloc(this); + INT64 imm = con->LngValue(); + emitAttr size = EA_8BYTE; + + if ((imm >= -256) && (imm < 1024)) + { + costSz = 2; + costEx = 1; + } + else if (iconNeedsReloc) + { + costSz = 8; + costEx = 2; + } + else + { + // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword + // There are three forms + // movk which loads into any halfword preserving the remaining halfwords + // movz which loads into any halfword zeroing the remaining halfwords + // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting the register + // In some cases it is preferable to use movn, because it has the side effect of filling the other halfwords + // with ones + + // Determine whether movn or movz will require the fewest instructions to populate the immediate + bool preferMovz = false; + bool preferMovn = false; + int instructionCount = 4; + + for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) + { + if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) + { + preferMovz = true; // by using a movk to start we can save one instruction + instructionCount--; + } + else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) + { + preferMovn = true; // by using a movn to start we can save one instruction + instructionCount--; + } + } + + costSz = 4 * instructionCount; + costEx = instructionCount; + } + } goto COMMON_CNS; } #elif defined(TARGET_ARM64) - case GT_CNS_STR: - costSz = 10; - costEx = 1; - goto COMMON_CNS; + case GT_CNS_STR: case GT_CNS_LNG: case GT_CNS_INT: - { - GenTreeIntConCommon* con = tree->AsIntConCommon(); - bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - INT64 imm = con->LngValue(); - emitAttr size = EA_8BYTE; - - if ((imm >= -256) && (imm < 1024)) + if (JitConfig.JitDisableConstCSE() == 1) { - costSz = 2; - costEx = 1; - } - else if (emitter::emitIns_valid_imm_for_mov(imm, size)) - { - costSz = 4; + costSz = 1; costEx = 1; } else { - // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword - // There are three forms - // movk which loads into any halfword preserving the remaining halfwords - // movz which loads into any halfword zeroing the remaining halfwords - // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting the register - // In some cases it is preferable to use movn, because it has the side effect of filling the other halfwords - // with ones - - // Determine whether movn or movz will require the fewest instructions to populate the immediate - bool preferMovz = false; - bool preferMovn = false; - int instructionCount = 4; + GenTreeIntConCommon* con = tree->AsIntConCommon(); + bool iconNeedsReloc = con->ImmedValNeedsReloc(this); + INT64 imm = con->LngValue(); + emitAttr size = EA_8BYTE; - for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) + if ((imm >= -256) && (imm < 1024)) { - if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) - { - preferMovz = true; // by using a movk to start we can save one instruction - instructionCount--; - } - else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) + costSz = 2; + costEx = 1; + } + else if (emitter::emitIns_valid_imm_for_mov(imm, size)) + { + costSz = 4; + costEx = 1; + } + else if (iconNeedsReloc) + { + costSz = 8; + costEx = 2; + } + else + { + // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword + // There are three forms + // movk which loads into any halfword preserving the remaining halfwords + // movz which loads into any halfword zeroing the remaining halfwords + // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting the register + // In some cases it is preferable to use movn, because it has the side effect of filling the other halfwords + // with ones + + // Determine whether movn or movz will require the fewest instructions to populate the immediate + bool preferMovz = false; + bool preferMovn = false; + int instructionCount = 4; + + for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) { - preferMovn = true; // by using a movn to start we can save one instruction - instructionCount--; + if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) + { + preferMovz = true; // by using a movk to start we can save one instruction + instructionCount--; + } + else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) + { + preferMovn = true; // by using a movn to start we can save one instruction + instructionCount--; + } } - } - costSz = 4 * instructionCount; - costEx = instructionCount; + costSz = 4 * instructionCount; + costEx = instructionCount; + } } goto COMMON_CNS; - } #else case GT_CNS_STR: diff --git a/src/coreclr/src/jit/jitconfigvalues.h b/src/coreclr/src/jit/jitconfigvalues.h index e6c1ab307e460a..9e08bd13da2eee 100644 --- a/src/coreclr/src/jit/jitconfigvalues.h +++ b/src/coreclr/src/jit/jitconfigvalues.h @@ -285,6 +285,10 @@ CONFIG_INTEGER(JitDisableSimdVN, W("JitDisableSimdVN"), 0) // Default 0, ValueNu // If 3, disable both SIMD and HW Intrinsic nodes #endif // FEATURE_SIMD +CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) // Default 0, We CSE Const including nearby with small offset + // If 1, then disable all CSE of Const + // If 2, then disable the CSE of Const with small offset + // If 3, then change the weighting of Const on x64 to match Arm64 /// /// JIT /// From e7787812e4b59e425d45a535adf37440d75fb8aa Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 21:23:00 -0700 Subject: [PATCH 12/30] Don't auto retry when using the AltJit as that results in two copies of every method in the output file --- src/coreclr/src/zap/zapinfo.cpp | 49 ++++++++++++++++++++------------- 1 file changed, 30 insertions(+), 19 deletions(-) diff --git a/src/coreclr/src/zap/zapinfo.cpp b/src/coreclr/src/zap/zapinfo.cpp index 310c0b93cb053e..c33e1cd0d8b585 100644 --- a/src/coreclr/src/zap/zapinfo.cpp +++ b/src/coreclr/src/zap/zapinfo.cpp @@ -502,6 +502,8 @@ void ZapInfo::CompileMethod() ULONG cCode; #ifdef ALLOW_SXS_JIT_NGEN + bool expectedAltJitFailure = false; + if (m_zapper->m_alternateJit) { res = m_zapper->m_alternateJit->compileMethod( this, @@ -509,43 +511,52 @@ void ZapInfo::CompileMethod() CORJIT_FLAGS::CORJIT_FLAG_CALL_GETJITFLAGS, &pCode, &cCode); + if ((res == CORJIT_SKIPPED) || SUCCEEDED(res)) + { + expectedAltJitFailure = true; + } if (FAILED(res)) { // We will fall back to the "main" JIT on failure. ResetForJitRetry(); } } + if (!expectedAltJitFailure) #endif // ALLOW_SXS_JIT_NGEN - - if (FAILED(res)) { - ICorJitCompiler * pCompiler = m_zapper->m_pJitCompiler; - res = pCompiler->compileMethod(this, - &m_currentMethodInfo, - CORJIT_FLAGS::CORJIT_FLAG_CALL_GETJITFLAGS, - &pCode, - &cCode); - if (FAILED(res)) { - ThrowExceptionForJitResult(res); + ICorJitCompiler* pCompiler = m_zapper->m_pJitCompiler; + res = pCompiler->compileMethod(this, + &m_currentMethodInfo, + CORJIT_FLAGS::CORJIT_FLAG_CALL_GETJITFLAGS, + &pCode, + &cCode); + + if (FAILED(res)) + { + ThrowExceptionForJitResult(res); + } } } MethodCompileComplete(m_currentMethodInfo.ftn); -#ifdef TARGET_X86 - // The x86 JIT over estimates the code size. Trim the blob size down to - // the actual size. - // We can do this only for non-split code. Adjusting the code size for split - // methods would hose offsets in GC info. - if (m_pColdCode == NULL) + if (!expectedAltJitFailure) { - m_pCode->AdjustBlobSize(cCode); - } +#ifdef TARGET_X86 + // The x86 JIT over estimates the code size. Trim the blob size down to + // the actual size. + // We can do this only for non-split code. Adjusting the code size for split + // methods would hose offsets in GC info. + if (m_pColdCode == NULL) + { + m_pCode->AdjustBlobSize(cCode); + } #endif - PublishCompiledMethod(); + PublishCompiledMethod(); + } } #ifndef FEATURE_FULL_NGEN From 84a208cc39189605dd543dbbb271dd28ac076777 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 20 May 2020 22:08:20 -0700 Subject: [PATCH 13/30] Implementation of CSE for GT_CNS_INT using a single CSE for constant values that differ only in the low 12 bits. --- src/coreclr/src/jit/compiler.h | 4 +- src/coreclr/src/jit/optcse.cpp | 188 +++++++++++++++++++++++++++------ 2 files changed, 157 insertions(+), 35 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 32ce7ebed5b7c3..6d9de051ccc27b 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6320,7 +6320,9 @@ class Compiler { CSEdsc* csdNextInBucket; // used by the hash table - INT64 csdHashKey; // the orginal hashkey + ssize_t csdHashKey; // the orginal hashkey + ssize_t csdConstDefValue; // When we CSE similar constants this is the value that we use as the def + ValueNum csdConstDefVN; // When we CSE similar constants this is the ValueNumber that we use for the LclVar assignment unsigned csdIndex; // 1..optCSECandidateCount bool csdLiveAcrossCall; diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 4b90b9a5adca83..3164627cdb59ed 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -401,10 +401,24 @@ void Compiler::optValnumCSE_Init() // unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { - size_t key; + ssize_t key; unsigned hash; unsigned hval; CSEdsc* hashDsc; + bool isIntConstHash = false; + bool enableConstCSE = false; + +#if defined(TARGET_ARM64) + if (JitConfig.JitDisableConstCSE() != 1) + { + enableConstCSE = true; + } +#else + if (JitConfig.JitDisableConstCSE() == -1) + { + enableConstCSE = true; + } +#endif // We use the liberal Value numbers when building the set of CSE ValueNum vnLib = tree->GetVN(VNK_Liberal); @@ -446,11 +460,11 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // if (vnOp2Lib != vnLib) { - key = (size_t)vnLib; // include the exc set in the hash key + key = (ssize_t)vnLib; // include the exc set in the hash key } else { - key = (size_t)vnLibNorm; + key = (ssize_t)vnLibNorm; } // If we didn't do the above we would have op1 as the CSE def @@ -459,9 +473,26 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // assert(vnLibNorm == vnStore->VNNormalValue(vnOp2Lib)); } - else // Not a GT_COMMA + else if (enableConstCSE && (tree->OperGet() == GT_CNS_INT) && vnStore->IsVNConstant(vnLibNorm)) + { + key = vnStore->CoercedConstantValue(vnLibNorm); + + // We can't share small offset constants when we require a reloc + if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && + (JitConfig.JitDisableConstCSE() != 2)) + { + // This will zero the upper 12 bits + key =(ssize_t) (((size_t) key) >> 12); + } + assert(key > 0); + + // We use negative values for 'key' as the flag + // that we are hashing constants (with a 12-bit offset) + key = -key; + } + else // Not a GT_COMMA or a GT_CNS_INT { - key = (size_t)vnLibNorm; + key = (ssize_t)vnLibNorm; } // Compute the hash value for the expression @@ -481,7 +512,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) for (hashDsc = optCSEhash[hval]; hashDsc; hashDsc = hashDsc->csdNextInBucket) { - if (hashDsc->csdHashKey == (INT64) key) + if (hashDsc->csdHashKey == (ssize_t) key) { treeStmtLst* newElem; @@ -587,7 +618,9 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { hashDsc = new (this, CMK_CSE) CSEdsc; - hashDsc->csdHashKey = (INT64) key; + hashDsc->csdHashKey = (ssize_t) key; + hashDsc->csdConstDefValue = 0; + hashDsc->csdConstDefVN = vnStore->VNForNull(); // uninit value hashDsc->csdIndex = 0; hashDsc->csdLiveAcrossCall = false; hashDsc->csdDefCount = 0; @@ -648,8 +681,17 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) #ifdef DEBUG if (verbose) { - printf("\nCSE candidate #%02u, vn=", CSEindex); - vnPrint((ValueNum) key, 0); + printf("\nCSE candidate #%02u, key=", CSEindex); + if (key >= 0) + { + vnPrint((unsigned)key, 0); + } + else + { + INT64 kVal = (-key) << 12; + printf("K_%012I64x", kVal); + } + printf(" in " FMT_BB ", [cost=%2u, size=%2u]: \n", compCurBB->bbNum, tree->GetCostEx(), tree->GetCostSz()); gtDispTree(tree); } @@ -704,15 +746,17 @@ unsigned Compiler::optValnumCSE_Locate() continue; } - // Don't CSE constant values, instead let the Value Number - // based Assertion Prop phase handle them. Here, unlike - // the rest of optCSE, we use the conservative value number + // We want to CSE simple constant leaf nodes, but we don't want to + // CSE non-leaf trees that compute CSE constant values. + // Instead we let the Value Number based Assertion Prop phase handle them. + // + // Here, unlike the rest of optCSE, we use the conservative value number // rather than the liberal one, since the conservative one // is what the Value Number based Assertion Prop will use // and the point is to avoid optimizing cases that it will // handle. // - if (vnStore->IsVNConstant(vnStore->VNConservativeNormalValue(tree->gtVNPair))) + if (!tree->OperIsLeaf() && vnStore->IsVNConstant(vnStore->VNConservativeNormalValue(tree->gtVNPair))) { continue; } @@ -1897,9 +1941,19 @@ class CSE_Heuristic cost = dsc->csdTree->GetCostEx(); } - printf("CSE #%02u, {$%-3x, $%-3x} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", - dsc->csdIndex, dsc->csdHashKey, dsc->defExcSetPromise, dsc->csdUseCount, def, use, cost, - dsc->csdLiveAcrossCall ? ", call" : " "); + if (dsc->csdHashKey >= 0) + { + printf("CSE #%02u, {$%-3x, $%-3x} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", + dsc->csdIndex, dsc->csdHashKey, dsc->defExcSetPromise, dsc->csdUseCount, def, use, cost, + dsc->csdLiveAcrossCall ? ", call" : " "); + } + else + { + INT64 kVal = (-dsc->csdHashKey) >> 12; + printf("CSE #%02u, {K_%012I64x} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", + dsc->csdIndex, kVal, dsc->csdUseCount, def, use, cost, + dsc->csdLiveAcrossCall ? ", call" : " "); + } m_pCompiler->gtDispTree(expr, nullptr, nullptr, true); } @@ -2688,6 +2742,10 @@ class CSE_Heuristic ValueNum currVN; bool setRefCnt = true; bool allSame = true; + bool isConstCSE = (dsc->csdHashKey < 0); + + BasicBlock::weight_t maxWeight = 0; + dsc->csdConstDefValue = -1; lst = dsc->csdTreeList; while (lst != nullptr) @@ -2708,6 +2766,17 @@ class CSE_Heuristic allSame = false; } + if (isConstCSE) + { + BasicBlock::weight_t curWeight = lst->tslBlock->getBBWeight(m_pCompiler); + if ((curWeight > maxWeight) || (dsc->csdConstDefValue == -1)) + { + maxWeight = curWeight; + dsc->csdConstDefValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); + dsc->csdConstDefVN = currVN; + } + } + BasicBlock* blk = lst->tslBlock; const BasicBlock::weight_t weight = blk->getBBWeight(m_pCompiler); @@ -2734,7 +2803,7 @@ class CSE_Heuristic } #ifdef DEBUG - if (!allSame) + if (!allSame && !isConstCSE) { lst = dsc->csdTreeList; GenTree* firstTree = lst->tslTree; @@ -2788,7 +2857,9 @@ class CSE_Heuristic // The cseLclVarType must be a compatible with expTyp // - noway_assert(IsCompatibleType(cseLclVarTyp, expTyp)); + ValueNumStore* vnStore = m_pCompiler->vnStore; + noway_assert(IsCompatibleType(cseLclVarTyp, expTyp) || (dsc->csdConstDefVN != vnStore->VNForNull())); + // This will contain the replacement tree for exp // It will either be the CSE def or CSE ref @@ -2816,12 +2887,27 @@ class CSE_Heuristic // We will replace the CSE ref with a new tree // this is typically just a simple use of the new CSE LclVar // - ValueNumStore* vnStore = m_pCompiler->vnStore; - cse = m_pCompiler->gtNewLclvNode(cseLclVarNum, cseLclVarTyp); - cse->SetDoNotCSE(); - // Assign the ssa num for the use. Note it may be the reserved num. - cse->AsLclVarCommon()->SetSsaNum(cseSsaNum); + // Create a reference to the CSE temp + GenTree* cseLclVar = m_pCompiler->gtNewLclvNode(cseLclVarNum, cseLclVarTyp); + cseLclVar->gtVNPair.SetBoth(dsc->csdConstDefVN); + + // Assign the ssa num for the lclvar use. Note it may be the reserved num. + cseLclVar->AsLclVarCommon()->SetSsaNum(cseSsaNum); + + cse = cseLclVar; + if (isConstCSE) + { + ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); + ssize_t curValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); + ssize_t delta = curValue - dsc->csdConstDefValue; + if (delta != 0) + { + GenTree* deltaNode = m_pCompiler->gtNewIconNode(delta, cseLclVarTyp); + cse = m_pCompiler->gtNewOperNode(GT_ADD, cseLclVarTyp, cseLclVar, deltaNode); + cse->SetDoNotCSE(); + } + } // assign the proper ValueNumber, A CSE use discards any exceptions cse->gtVNPair = vnStore->VNPNormalPair(exp->gtVNPair); @@ -2907,7 +2993,6 @@ class CSE_Heuristic GenTree* cseVal = cse; GenTree* curSideEff = sideEffList; - ValueNumStore* vnStore = m_pCompiler->vnStore; ValueNumPair exceptions_vnp = ValueNumStore::VNPForEmptyExcSet(); while ((curSideEff->OperGet() == GT_COMMA) || (curSideEff->OperGet() == GT_ASG)) @@ -2963,6 +3048,17 @@ class CSE_Heuristic exp->gtCSEnum = NO_CSE; // clear the gtCSEnum field GenTree* val = exp; + if (isConstCSE) + { + ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); + ssize_t curValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); + ssize_t delta = curValue - dsc->csdConstDefValue; + if (delta != 0) + { + val = m_pCompiler->gtNewIconNode(dsc->csdConstDefValue, cseLclVarTyp); + val->gtVNPair.SetBoth(dsc->csdConstDefVN); + } + } /* Create an assignment of the value to the temp */ GenTree* asg = m_pCompiler->gtNewTempAssign(cseLclVarNum, val); @@ -3004,16 +3100,30 @@ class CSE_Heuristic } /* Create a reference to the CSE temp */ - GenTree* ref = m_pCompiler->gtNewLclvNode(cseLclVarNum, cseLclVarTyp); - ref->gtVNPair = val->gtVNPair; // The new 'ref' is the same as 'val' - ref->SetDoNotCSE(); + GenTree* cseLclVar = m_pCompiler->gtNewLclvNode(cseLclVarNum, cseLclVarTyp); + cseLclVar->gtVNPair.SetBoth(dsc->csdConstDefVN); - // Assign the ssa num for the ref use. Note it may be the reserved num. - ref->AsLclVarCommon()->SetSsaNum(cseSsaNum); + // Assign the ssa num for the lclvar use. Note it may be the reserved num. + cseLclVar->AsLclVarCommon()->SetSsaNum(cseSsaNum); + + GenTree* cseUse = cseLclVar; + if (isConstCSE) + { + ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); + ssize_t curValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); + ssize_t delta = curValue - dsc->csdConstDefValue; + if (delta != 0) + { + GenTree* deltaNode = m_pCompiler->gtNewIconNode(delta, cseLclVarTyp); + cseUse = m_pCompiler->gtNewOperNode(GT_ADD, cseLclVarTyp, cseLclVar, deltaNode); + cseUse->SetDoNotCSE(); + } + } + cseUse->gtVNPair = val->gtVNPair; // The 'cseUse' is equal to 'val' /* Create a comma node for the CSE assignment */ - cse = m_pCompiler->gtNewOperNode(GT_COMMA, expTyp, origAsg, ref); - cse->gtVNPair = ref->gtVNPair; // The comma's value is the same as 'val' + cse = m_pCompiler->gtNewOperNode(GT_COMMA, expTyp, origAsg, cseUse); + cse->gtVNPair = cseUse->gtVNPair; // The comma's value is the same as 'val' // as the assignment to the CSE LclVar // cannot add any new exceptions } @@ -3100,9 +3210,19 @@ class CSE_Heuristic #ifdef DEBUG if (m_pCompiler->verbose) { - printf("\nConsidering CSE #%02u {$%-3x, $%-3x} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), - dsc->csdHashKey, dsc->defExcSetPromise, candidate.DefCount(), candidate.UseCount(), - candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); + if (dsc->csdHashKey >= 0) + { + printf("\nConsidering CSE #%02u {$%-3x, $%-3x} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), + dsc->csdHashKey, dsc->defExcSetPromise, candidate.DefCount(), candidate.UseCount(), + candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); + } + else + { + INT64 kVal = (-dsc->csdHashKey) >> 12; + printf("\nConsidering CSE #%02u {K_%012I64x} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), + kVal, candidate.DefCount(), candidate.UseCount(), + candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); + } printf("CSE Expression : \n"); m_pCompiler->gtDispTree(candidate.Expr()); printf("\n"); From d5ccbdb6c4259ea8c69ea18e02c0703191e0f905 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 12 Jun 2020 13:49:37 -0700 Subject: [PATCH 14/30] Fix for JitDisableConstCSE() == 3 --- src/coreclr/src/jit/compiler.h | 5 +- src/coreclr/src/jit/gentree.cpp | 45 +++++++++-------- src/coreclr/src/jit/gentree.h | 2 +- src/coreclr/src/jit/jitconfigvalues.h | 5 +- src/coreclr/src/jit/optcse.cpp | 70 +++++++++++++-------------- 5 files changed, 65 insertions(+), 62 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 6d9de051ccc27b..8ee88db678a684 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6320,9 +6320,10 @@ class Compiler { CSEdsc* csdNextInBucket; // used by the hash table - ssize_t csdHashKey; // the orginal hashkey + ssize_t csdHashKey; // the orginal hashkey ssize_t csdConstDefValue; // When we CSE similar constants this is the value that we use as the def - ValueNum csdConstDefVN; // When we CSE similar constants this is the ValueNumber that we use for the LclVar assignment + ValueNum csdConstDefVN; // When we CSE similar constants this is the ValueNumber that we use for the LclVar + // assignment unsigned csdIndex; // 1..optCSECandidateCount bool csdLiveAcrossCall; diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index d603221517639c..654cadfc0e8e09 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3306,16 +3306,16 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_CNS_LNG: case GT_CNS_INT: { - GenTreeIntConCommon* con = tree->AsIntConCommon(); - ssize_t conVal = (oper == GT_CNS_LNG) ? (ssize_t)con->LngValue() : con->IconValue(); - bool fitsInVal = true; + GenTreeIntConCommon* con = tree->AsIntConCommon(); + ssize_t conVal = (oper == GT_CNS_LNG) ? (ssize_t)con->LngValue() : con->IconValue(); + bool fitsInVal = true; #ifdef TARGET_X86 if (oper == GT_CNS_LNG) { INT64 lngVal = con->LngValue(); - conVal = (ssize_t)lngVal; // truncate to 32-bits + conVal = (ssize_t)lngVal; // truncate to 32-bits fitsInVal = ((INT64)conVal == lngVal); } @@ -3358,10 +3358,10 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) if (JitConfig.JitDisableConstCSE() == 3) { - GenTreeIntConCommon* con = tree->AsIntConCommon(); - bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - INT64 imm = con->LngValue(); - emitAttr size = EA_8BYTE; + GenTreeIntConCommon* con = tree->AsIntConCommon(); + bool iconNeedsReloc = con->ImmedValNeedsReloc(this); + INT64 imm = con->LngValue(); + emitAttr size = EA_8BYTE; if ((imm >= -256) && (imm < 1024)) { @@ -3379,13 +3379,15 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // There are three forms // movk which loads into any halfword preserving the remaining halfwords // movz which loads into any halfword zeroing the remaining halfwords - // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting the register - // In some cases it is preferable to use movn, because it has the side effect of filling the other halfwords + // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting + // the register + // In some cases it is preferable to use movn, because it has the side effect of filling the + // other halfwords // with ones // Determine whether movn or movz will require the fewest instructions to populate the immediate - bool preferMovz = false; - bool preferMovn = false; + bool preferMovz = false; + bool preferMovn = false; int instructionCount = 4; for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) @@ -3421,10 +3423,10 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } else { - GenTreeIntConCommon* con = tree->AsIntConCommon(); - bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - INT64 imm = con->LngValue(); - emitAttr size = EA_8BYTE; + GenTreeIntConCommon* con = tree->AsIntConCommon(); + bool iconNeedsReloc = con->ImmedValNeedsReloc(this); + INT64 imm = con->LngValue(); + emitAttr size = EA_8BYTE; if ((imm >= -256) && (imm < 1024)) { @@ -3447,13 +3449,15 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // There are three forms // movk which loads into any halfword preserving the remaining halfwords // movz which loads into any halfword zeroing the remaining halfwords - // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting the register - // In some cases it is preferable to use movn, because it has the side effect of filling the other halfwords + // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting + // the register + // In some cases it is preferable to use movn, because it has the side effect of filling the + // other halfwords // with ones // Determine whether movn or movz will require the fewest instructions to populate the immediate - bool preferMovz = false; - bool preferMovn = false; + bool preferMovz = false; + bool preferMovn = false; int instructionCount = 4; for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) @@ -6903,7 +6907,6 @@ bool GenTreeOp::usesMagicNumberDivision(Compiler* comp) } } - const bool isDiv = OperIs(GT_DIV, GT_UDIV); if (isDiv) diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 0f7a1e98001b29..9f0dfca2fb4383 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -636,7 +636,7 @@ struct GenTree void ClearRegNum() { _gtRegNum = REG_NA; - INDEBUG(gtRegTag = GT_REGTAG_NONE;) + INDEBUG(gtRegTag = GT_REGTAG_NONE;) } // Copy the _gtRegNum/gtRegTag fields diff --git a/src/coreclr/src/jit/jitconfigvalues.h b/src/coreclr/src/jit/jitconfigvalues.h index 9e08bd13da2eee..4c3c52de2e026e 100644 --- a/src/coreclr/src/jit/jitconfigvalues.h +++ b/src/coreclr/src/jit/jitconfigvalues.h @@ -285,10 +285,11 @@ CONFIG_INTEGER(JitDisableSimdVN, W("JitDisableSimdVN"), 0) // Default 0, ValueNu // If 3, disable both SIMD and HW Intrinsic nodes #endif // FEATURE_SIMD -CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) // Default 0, We CSE Const including nearby with small offset +CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) // Default 0, We CSE Const including nearby with small + // offset // If 1, then disable all CSE of Const // If 2, then disable the CSE of Const with small offset - // If 3, then change the weighting of Const on x64 to match Arm64 +// If 3, then change the weighting of Const on x64 to match Arm64 /// /// JIT /// diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 3164627cdb59ed..c349301672b588 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -414,7 +414,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) enableConstCSE = true; } #else - if (JitConfig.JitDisableConstCSE() == -1) + if (JitConfig.JitDisableConstCSE() == 3) { enableConstCSE = true; } @@ -478,11 +478,10 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) key = vnStore->CoercedConstantValue(vnLibNorm); // We can't share small offset constants when we require a reloc - if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && - (JitConfig.JitDisableConstCSE() != 2)) + if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && (JitConfig.JitDisableConstCSE() != 2)) { // This will zero the upper 12 bits - key =(ssize_t) (((size_t) key) >> 12); + key = (ssize_t)(((size_t)key) >> 12); } assert(key > 0); @@ -512,7 +511,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) for (hashDsc = optCSEhash[hval]; hashDsc; hashDsc = hashDsc->csdNextInBucket) { - if (hashDsc->csdHashKey == (ssize_t) key) + if (hashDsc->csdHashKey == (ssize_t)key) { treeStmtLst* newElem; @@ -618,7 +617,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { hashDsc = new (this, CMK_CSE) CSEdsc; - hashDsc->csdHashKey = (ssize_t) key; + hashDsc->csdHashKey = (ssize_t)key; hashDsc->csdConstDefValue = 0; hashDsc->csdConstDefVN = vnStore->VNForNull(); // uninit value hashDsc->csdIndex = 0; @@ -1951,8 +1950,8 @@ class CSE_Heuristic { INT64 kVal = (-dsc->csdHashKey) >> 12; printf("CSE #%02u, {K_%012I64x} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", - dsc->csdIndex, kVal, dsc->csdUseCount, def, use, cost, - dsc->csdLiveAcrossCall ? ", call" : " "); + dsc->csdIndex, kVal, dsc->csdUseCount, def, use, cost, + dsc->csdLiveAcrossCall ? ", call" : " "); } m_pCompiler->gtDispTree(expr, nullptr, nullptr, true); @@ -2740,12 +2739,12 @@ class CSE_Heuristic // ValueNum firstVN = ValueNumStore::NoVN; ValueNum currVN; - bool setRefCnt = true; - bool allSame = true; + bool setRefCnt = true; + bool allSame = true; bool isConstCSE = (dsc->csdHashKey < 0); - BasicBlock::weight_t maxWeight = 0; - dsc->csdConstDefValue = -1; + BasicBlock::weight_t maxWeight = 0; + dsc->csdConstDefValue = -1; lst = dsc->csdTreeList; while (lst != nullptr) @@ -2768,16 +2767,16 @@ class CSE_Heuristic if (isConstCSE) { - BasicBlock::weight_t curWeight = lst->tslBlock->getBBWeight(m_pCompiler); + BasicBlock::weight_t curWeight = lst->tslBlock->getBBWeight(m_pCompiler); if ((curWeight > maxWeight) || (dsc->csdConstDefValue == -1)) { - maxWeight = curWeight; + maxWeight = curWeight; dsc->csdConstDefValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); - dsc->csdConstDefVN = currVN; + dsc->csdConstDefVN = currVN; } } - BasicBlock* blk = lst->tslBlock; + BasicBlock* blk = lst->tslBlock; const BasicBlock::weight_t weight = blk->getBBWeight(m_pCompiler); if (setRefCnt) @@ -2860,7 +2859,6 @@ class CSE_Heuristic ValueNumStore* vnStore = m_pCompiler->vnStore; noway_assert(IsCompatibleType(cseLclVarTyp, expTyp) || (dsc->csdConstDefVN != vnStore->VNForNull())); - // This will contain the replacement tree for exp // It will either be the CSE def or CSE ref // @@ -2904,7 +2902,7 @@ class CSE_Heuristic if (delta != 0) { GenTree* deltaNode = m_pCompiler->gtNewIconNode(delta, cseLclVarTyp); - cse = m_pCompiler->gtNewOperNode(GT_ADD, cseLclVarTyp, cseLclVar, deltaNode); + cse = m_pCompiler->gtNewOperNode(GT_ADD, cseLclVarTyp, cseLclVar, deltaNode); cse->SetDoNotCSE(); } } @@ -2991,9 +2989,9 @@ class CSE_Heuristic } #endif - GenTree* cseVal = cse; - GenTree* curSideEff = sideEffList; - ValueNumPair exceptions_vnp = ValueNumStore::VNPForEmptyExcSet(); + GenTree* cseVal = cse; + GenTree* curSideEff = sideEffList; + ValueNumPair exceptions_vnp = ValueNumStore::VNPForEmptyExcSet(); while ((curSideEff->OperGet() == GT_COMMA) || (curSideEff->OperGet() == GT_ASG)) { @@ -3050,9 +3048,9 @@ class CSE_Heuristic GenTree* val = exp; if (isConstCSE) { - ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); + ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); ssize_t curValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); - ssize_t delta = curValue - dsc->csdConstDefValue; + ssize_t delta = curValue - dsc->csdConstDefValue; if (delta != 0) { val = m_pCompiler->gtNewIconNode(dsc->csdConstDefValue, cseLclVarTyp); @@ -3109,13 +3107,13 @@ class CSE_Heuristic GenTree* cseUse = cseLclVar; if (isConstCSE) { - ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); + ValueNum currVN = m_pCompiler->vnStore->VNLiberalNormalValue(exp->gtVNPair); ssize_t curValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); - ssize_t delta = curValue - dsc->csdConstDefValue; + ssize_t delta = curValue - dsc->csdConstDefValue; if (delta != 0) { GenTree* deltaNode = m_pCompiler->gtNewIconNode(delta, cseLclVarTyp); - cseUse = m_pCompiler->gtNewOperNode(GT_ADD, cseLclVarTyp, cseLclVar, deltaNode); + cseUse = m_pCompiler->gtNewOperNode(GT_ADD, cseLclVarTyp, cseLclVar, deltaNode); cseUse->SetDoNotCSE(); } } @@ -3124,12 +3122,12 @@ class CSE_Heuristic /* Create a comma node for the CSE assignment */ cse = m_pCompiler->gtNewOperNode(GT_COMMA, expTyp, origAsg, cseUse); cse->gtVNPair = cseUse->gtVNPair; // The comma's value is the same as 'val' - // as the assignment to the CSE LclVar - // cannot add any new exceptions + // as the assignment to the CSE LclVar + // cannot add any new exceptions } - cse->CopyReg(exp); // The cse inheirits any reg num property from the orginal exp node - exp->ClearRegNum(); // The exp node (for a CSE def) no longer has a register requirement + cse->CopyReg(exp); // The cse inheirits any reg num property from the orginal exp node + exp->ClearRegNum(); // The exp node (for a CSE def) no longer has a register requirement // Walk the statement 'stmt' and find the pointer // in the tree is pointing to 'exp' @@ -3212,16 +3210,16 @@ class CSE_Heuristic { if (dsc->csdHashKey >= 0) { - printf("\nConsidering CSE #%02u {$%-3x, $%-3x} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), - dsc->csdHashKey, dsc->defExcSetPromise, candidate.DefCount(), candidate.UseCount(), - candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); + printf("\nConsidering CSE #%02u {$%-3x, $%-3x} [def=%3u, use=%3u, cost=%3u%s]\n", + candidate.CseIndex(), dsc->csdHashKey, dsc->defExcSetPromise, candidate.DefCount(), + candidate.UseCount(), candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); } else { INT64 kVal = (-dsc->csdHashKey) >> 12; - printf("\nConsidering CSE #%02u {K_%012I64x} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), - kVal, candidate.DefCount(), candidate.UseCount(), - candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); + printf("\nConsidering CSE #%02u {K_%012I64x} [def=%3u, use=%3u, cost=%3u%s]\n", + candidate.CseIndex(), kVal, candidate.DefCount(), candidate.UseCount(), candidate.Cost(), + dsc->csdLiveAcrossCall ? ", call" : " "); } printf("CSE Expression : \n"); m_pCompiler->gtDispTree(candidate.Expr()); From 5b8b9bdaecc8cf0d97e71ff72e79e5210a021c9d Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 17 Jun 2020 13:43:03 -0700 Subject: [PATCH 15/30] In CSE added check for mismatched types on GT_CNS_INT nodes --- src/coreclr/src/jit/optcse.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index c349301672b588..875892ffe329fb 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -513,6 +513,12 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { if (hashDsc->csdHashKey == (ssize_t)key) { + // Check for mismatched types on GT_CNS_INT nodes + if ((tree->OperGet() == GT_CNS_INT) && (tree->TypeGet() != hashDsc->csdTree->TypeGet())) + { + continue; + } + treeStmtLst* newElem; /* Have we started the list of matching nodes? */ From 5a8fac2c8be61749608a18bff83f0ee71559a592 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Mon, 22 Jun 2020 18:06:49 -0700 Subject: [PATCH 16/30] Fix assert divMod->markedMagicNumberDivision() --- src/coreclr/src/jit/gentree.cpp | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 654cadfc0e8e09..7ddd62c78c6c58 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -6836,8 +6836,8 @@ bool GenTreeOp::usesMagicNumberDivision(Compiler* comp) #endif // TARGET_ARM64 bool isSignedDivide = OperIs(GT_DIV, GT_MOD); - GenTree* dividend = gtGetOp1(); - GenTree* divisor = gtGetOp2(); + GenTree* dividend = gtGetOp1()->gtEffectiveVal(/*commaOnly*/ true); + GenTree* divisor = gtGetOp2()->gtEffectiveVal(/*commaOnly*/ true); #if !defined(TARGET_64BIT) if (dividend->OperIs(GT_LONG)) @@ -6951,7 +6951,7 @@ void GenTreeOp::checkMagicNumberDivision(Compiler* comp) gtFlags |= GTF_DIV_USE_MAGIC; // Now set DONT_CSE on the GT_CNS_INT divisor - GenTree* divisor = gtGetOp2(); + GenTree* divisor = gtGetOp2()->gtEffectiveVal(/*commaOnly*/ true); divisor->gtFlags |= GTF_DONT_CSE; } } From 5effd8e5376a28fd887d6920a1573c26ce071020 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Tue, 23 Jun 2020 13:29:14 -0700 Subject: [PATCH 17/30] Propagate the gtFlags for a gtCallAddr node Const CSE may create an assignment node for the target of a calli: indirect call --- src/coreclr/src/jit/morph.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index d0e5fa272ba18d..0df4266c837ecc 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -3982,6 +3982,8 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* call) if (call->gtCallType == CT_INDIRECT) { call->gtCallAddr = fgMorphTree(call->gtCallAddr); + // Const CSE may create an assignment node here + flagsSummary |= call->gtCallAddr->gtFlags; } #if FEATURE_FIXED_OUT_ARGS From d6ed1b1a72296f4738aab97e45a4577aa00629b1 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 24 Jun 2020 17:59:15 -0700 Subject: [PATCH 18/30] Propagating a constant may create an opportunity to use a magic number division --- src/coreclr/src/jit/earlyprop.cpp | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/src/coreclr/src/jit/earlyprop.cpp b/src/coreclr/src/jit/earlyprop.cpp index 6c8a521fc75023..0a19768aef83bc 100644 --- a/src/coreclr/src/jit/earlyprop.cpp +++ b/src/coreclr/src/jit/earlyprop.cpp @@ -445,6 +445,14 @@ GenTree* Compiler::optEarlyPropRewriteTree(GenTree* tree, LocalNumberToNullCheck // actualValClone has small tree node size, it is safe to use CopyFrom here. tree->ReplaceWith(actualValClone, this); + // Propagating a constant may create an opportunity to use a magic number division + // + if ((tree->gtNext != nullptr) && tree->gtNext->OperIsBinary()) + { + // We need to mark the parent divide/mod operation when this occurs + tree->gtNext->AsOp()->checkMagicNumberDivision(this); + } + #ifdef DEBUG if (verbose) { From 640e9450bb10ff7d34e0d228e00e25bf1795b4ef Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 24 Jun 2020 18:04:47 -0700 Subject: [PATCH 19/30] Only perform conversion of floating point GT_DIV to an exact reciprocal GT_MUL when fgGlobalMorph is true --- src/coreclr/src/jit/morph.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index 0df4266c837ecc..ac1abd5202c641 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -11687,7 +11687,7 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) // Replace "val / dcon" with "val * (1.0 / dcon)" if dcon is a power of two. // Powers of two within range are always exactly represented, // so multiplication by the reciprocal is safe in this scenario - if (op2->IsCnsFltOrDbl()) + if (fgGlobalMorph && op2->IsCnsFltOrDbl()) { double divisor = op2->AsDblCon()->gtDconVal; if (((typ == TYP_DOUBLE) && FloatingPointUtils::hasPreciseReciprocal(divisor)) || From 2240392b5944e165bbe6bd2a4e96b8446ea2f04d Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 25 Jun 2020 11:44:45 -0700 Subject: [PATCH 20/30] Mark the non-standard virtual stub indirection address arg with GTF_DONT_CSE --- src/coreclr/src/jit/morph.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index ac1abd5202c641..e746b0b9cbd259 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -2707,6 +2707,8 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) indirectCellAddress->AsIntCon()->gtTargetHandle = (size_t)call->gtCallMethHnd; #endif indirectCellAddress->SetRegNum(REG_R2R_INDIRECT_PARAM); + // Don't attempt to CSE this constant + indirectCellAddress->SetDoNotCSE(); // Push the stub address onto the list of arguments. call->gtCallArgs = gtPrependNewCallArg(indirectCellAddress, call->gtCallArgs); From 4b429fd0333ee3c652f16e5bcc8358c9173565a1 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Thu, 25 Jun 2020 16:40:59 -0700 Subject: [PATCH 21/30] Fix the gtCost GT_CNS_LNG for ARM32 --- src/coreclr/src/jit/gentree.cpp | 39 ++++++++++++++++++++++++++++----- src/coreclr/src/jit/optcse.cpp | 6 +++++ 2 files changed, 39 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 7ddd62c78c6c58..59a81d40535969 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3250,17 +3250,44 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) switch (oper) { #ifdef TARGET_ARM - case GT_CNS_LNG: - costSz = 9; - costEx = 4; - goto COMMON_CNS; - case GT_CNS_STR: // Uses movw/movt costSz = 7; costEx = 3; goto COMMON_CNS; + case GT_CNS_LNG: + { + GenTreeIntConCommon* con = tree->AsIntConCommon(); + + INT64 lngVal = con->LngValue(); + INT32 loVal = (INT32)(lngVal & 0xffffffff); + bool fitsInVal = ((INT64)loVal == lngVal); + + if (!fitsInVal) + { + costSz = 9; + costEx = 4; + } + else if (!codeGen->validImmForInstr(INS_mov, (target_ssize_t)loVal)) + { + // Uses movw/movt + costSz = 8; + costEx = 3; + } + else if ((unsigned)loVal <= 0xff) + { + costSz = 2; + costEx = 2; + } + else + { + costSz = 4; + costEx = 2; + } + goto COMMON_CNS; + } + case GT_CNS_INT: { // If the constant is a handle then it will need to have a relocation @@ -3274,7 +3301,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) { // Uses movw/movt costSz = 7; - costEx = 3; + costEx = 2; } else if (((unsigned)tree->AsIntCon()->gtIconVal) <= 0x00ff) { diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 875892ffe329fb..bc032a918a90e3 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -2293,6 +2293,12 @@ class CSE_Heuristic // because it doesn't take into account that we might use a vector register for struct copies. slotCount = (size + TARGET_POINTER_SIZE - 1) / TARGET_POINTER_SIZE; } +#ifndef TARGET_64BIT + if (candidate->Expr()->TypeGet() == TYP_LONG) + { + slotCount = 2; // on 32-bit targets longs use two registers + } +#endif if (CodeOptKind() == Compiler::SMALL_CODE) { From 8340e81a0dd51a39430a6ee7aba01c0ea992565c Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Sat, 27 Jun 2020 12:05:11 -0700 Subject: [PATCH 22/30] Update arm32 GT_CNS costs. Update JitDisableConstCSE to support various options 1-5 --- src/coreclr/src/jit/gentree.cpp | 123 ++++++++++++++------------ src/coreclr/src/jit/jitconfigvalues.h | 14 +-- src/coreclr/src/jit/optcse.cpp | 53 ++++++++--- 3 files changed, 117 insertions(+), 73 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 59a81d40535969..683f516cca4387 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3252,38 +3252,45 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) #ifdef TARGET_ARM case GT_CNS_STR: // Uses movw/movt - costSz = 7; - costEx = 3; + costSz = 8; + costEx = 2; goto COMMON_CNS; case GT_CNS_LNG: { GenTreeIntConCommon* con = tree->AsIntConCommon(); - INT64 lngVal = con->LngValue(); - INT32 loVal = (INT32)(lngVal & 0xffffffff); - bool fitsInVal = ((INT64)loVal == lngVal); + INT64 lngVal = con->LngValue(); + INT32 loVal = (INT32)(lngVal & 0xffffffff); + INT32 hiVal = (INT32)(lngVal >> 32); - if (!fitsInVal) - { - costSz = 9; - costEx = 4; - } - else if (!codeGen->validImmForInstr(INS_mov, (target_ssize_t)loVal)) - { - // Uses movw/movt - costSz = 8; - costEx = 3; - } - else if ((unsigned)loVal <= 0xff) + if (lngVal == 0) { - costSz = 2; - costEx = 2; + costSz = 1; + costEx = 1; } else { - costSz = 4; - costEx = 2; + // Minimum of one instruction to setup hiVal, + // and one instruction to setup loVal + costSz = 4 + 4; + costEx = 1 + 1; + + if (!codeGen->validImmForInstr(INS_mov, (target_ssize_t)hiVal) && + !codeGen->validImmForInstr(INS_mvn, (target_ssize_t)hiVal)) + { + // Needs extra instruction: movw/movt + costSz += 4; + costEx += 1; + } + + if (!codeGen->validImmForInstr(INS_mov, (target_ssize_t)loVal) && + !codeGen->validImmForInstr(INS_mvn, (target_ssize_t)loVal)) + { + // Needs extra instruction: movw/movt + costSz += 4; + costEx += 1; + } } goto COMMON_CNS; } @@ -3295,26 +3302,33 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // Any constant that requires a reloc must use the movw/movt sequence // GenTreeIntConCommon* con = tree->AsIntConCommon(); + INT32 conVal = con->IconValue(); - if (con->ImmedValNeedsReloc(this) || - !codeGen->validImmForInstr(INS_mov, (target_ssize_t)tree->AsIntCon()->gtIconVal)) + if (con->ImmedValNeedsReloc(this)) { - // Uses movw/movt - costSz = 7; + // Requires movw/movt + costSz = 8; costEx = 2; } - else if (((unsigned)tree->AsIntCon()->gtIconVal) <= 0x00ff) + else if (codeGen->validImmForInstr(INS_add, (target_ssize_t)conVal)) { - // mov Rd, + // Typically included with parent oper costSz = 1; costEx = 1; } - else + else if (codeGen->validImmForInstr(INS_mov, (target_ssize_t)conVal) && + codeGen->validImmForInstr(INS_mvn, (target_ssize_t)conVal)) { - // Uses movw/mvn - costSz = 3; + // Uses mov ot mvn + costSz = 4; costEx = 1; } + else + { + // Needs movw/movt + costSz = 8; + costEx = 2; + } goto COMMON_CNS; } @@ -3383,23 +3397,28 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } #endif // TARGET_X86 - if (JitConfig.JitDisableConstCSE() == 3) + // If JitDisableConstCSE is set to 4 or 5 we change the costs of the GT_CNS_INT + // to match that used by ARM64, so that we get a similar set of CSE performed + // when running on x64 + // + int configValue = JitConfig.JitDisableConstCSE(); + if ((configValue == 4) || (configValue == 5)) { - GenTreeIntConCommon* con = tree->AsIntConCommon(); + GenTreeIntConCommon* con = tree->AsIntConCommon(); bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - INT64 imm = con->LngValue(); - emitAttr size = EA_8BYTE; + INT64 imm = con->LngValue(); + emitAttr size = EA_8BYTE; - if ((imm >= -256) && (imm < 1024)) - { - costSz = 2; - costEx = 1; - } - else if (iconNeedsReloc) + if (iconNeedsReloc) { costSz = 8; costEx = 2; } + else if ((imm >= -256) && (imm < 1024)) + { + costSz = 2; + costEx = 1; + } else { // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword @@ -3413,8 +3432,8 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // with ones // Determine whether movn or movz will require the fewest instructions to populate the immediate - bool preferMovz = false; - bool preferMovn = false; + bool preferMovz = false; + bool preferMovn = false; int instructionCount = 4; for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) @@ -3443,19 +3462,18 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_CNS_STR: case GT_CNS_LNG: case GT_CNS_INT: - if (JitConfig.JitDisableConstCSE() == 1) - { - costSz = 1; - costEx = 1; - } - else { GenTreeIntConCommon* con = tree->AsIntConCommon(); bool iconNeedsReloc = con->ImmedValNeedsReloc(this); INT64 imm = con->LngValue(); emitAttr size = EA_8BYTE; - if ((imm >= -256) && (imm < 1024)) + if (iconNeedsReloc) + { + costSz = 8; + costEx = 2; + } + else if ((imm >= -256) && (imm < 1024)) { costSz = 2; costEx = 1; @@ -3465,11 +3483,6 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) costSz = 4; costEx = 1; } - else if (iconNeedsReloc) - { - costSz = 8; - costEx = 2; - } else { // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword @@ -3501,8 +3514,8 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } } - costSz = 4 * instructionCount; costEx = instructionCount; + costSz = 4 * instructionCount; } } goto COMMON_CNS; diff --git a/src/coreclr/src/jit/jitconfigvalues.h b/src/coreclr/src/jit/jitconfigvalues.h index 4c3c52de2e026e..8ff5648ec3e218 100644 --- a/src/coreclr/src/jit/jitconfigvalues.h +++ b/src/coreclr/src/jit/jitconfigvalues.h @@ -285,11 +285,15 @@ CONFIG_INTEGER(JitDisableSimdVN, W("JitDisableSimdVN"), 0) // Default 0, ValueNu // If 3, disable both SIMD and HW Intrinsic nodes #endif // FEATURE_SIMD -CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) // Default 0, We CSE Const including nearby with small - // offset - // If 1, then disable all CSE of Const - // If 2, then disable the CSE of Const with small offset -// If 3, then change the weighting of Const on x64 to match Arm64 +// Default 0, enable the CSE of Constants, including nearby offsets. (only for ARM64) +// If 1, disable all the CSE of Constants +// If 2, enable the CSE of Constants but don't combine with nearby offsets. (only for ARM64) +// If 3, enable the CSE of Constants including nearby offsets. (all platforms) +// If 4, same as 3, but also change X64 weighting of const nodes to match ARM64 costs +// If 5, same as 4, but don't combine with nearby offsets. (all platforms) +// +CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) + /// /// JIT /// diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index bc032a918a90e3..d4fc88cb0b4a9b 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -405,20 +405,22 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) unsigned hash; unsigned hval; CSEdsc* hashDsc; - bool isIntConstHash = false; - bool enableConstCSE = false; + bool isIntConstHash = false; + bool enableSharedConstCSE = false; + int configValue = JitConfig.JitDisableConstCSE(); #if defined(TARGET_ARM64) - if (JitConfig.JitDisableConstCSE() != 1) + if (configValue != 2) { - enableConstCSE = true; + enableSharedConstCSE = true; } -#else - if (JitConfig.JitDisableConstCSE() == 3) +#endif // TARGET_ARM64 + + // All Platforms - don't combine with nearby offsets + if (configValue == 5) { - enableConstCSE = true; + enableSharedConstCSE = true; } -#endif // We use the liberal Value numbers when building the set of CSE ValueNum vnLib = tree->GetVN(VNK_Liberal); @@ -473,14 +475,17 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // assert(vnLibNorm == vnStore->VNNormalValue(vnOp2Lib)); } - else if (enableConstCSE && (tree->OperGet() == GT_CNS_INT) && vnStore->IsVNConstant(vnLibNorm)) + else if (enableSharedConstCSE && tree->IsIntegralConst()) { + assert(vnStore->IsVNConstant(vnLibNorm)); key = vnStore->CoercedConstantValue(vnLibNorm); - // We can't share small offset constants when we require a reloc - if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && (JitConfig.JitDisableConstCSE() != 2)) + // We don't shared small offset constants when we require a reloc + if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this)) { - // This will zero the upper 12 bits + // Make constants that have the same upper bits use the same key + + // Shift the key right by 12 bits key = (ssize_t)(((size_t)key) >> 12); } assert(key > 0); @@ -716,6 +721,21 @@ unsigned Compiler::optValnumCSE_Locate() { // Locate CSE candidates and assign them indices + bool disableConstCSE = false; + + int configValue = JitConfig.JitDisableConstCSE(); + + if (configValue == 1) + { + disableConstCSE = true; + } +#if !defined(TARGET_ARM64) + if (configValue == 0) + { + disableConstCSE = true; + } +#endif + for (BasicBlock* block = fgFirstBB; block != nullptr; block = block->bbNext) { /* Make the block publicly available */ @@ -741,6 +761,13 @@ unsigned Compiler::optValnumCSE_Locate() optCseUpdateCheckedBoundMap(tree); } + // Don't allow CSE of constants if it is disabled + // + if (disableConstCSE && tree->IsIntegralConst()) + { + continue; + } + if (!optIsCSEcandidate(tree)) { continue; @@ -2293,7 +2320,7 @@ class CSE_Heuristic // because it doesn't take into account that we might use a vector register for struct copies. slotCount = (size + TARGET_POINTER_SIZE - 1) / TARGET_POINTER_SIZE; } -#ifndef TARGET_64BIT +#if 0 //ndef TARGET_64BIT if (candidate->Expr()->TypeGet() == TYP_LONG) { slotCount = 2; // on 32-bit targets longs use two registers From 0d84d8c2282b9796d2eae95b364a1fdf788a18dd Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Mon, 29 Jun 2020 18:57:44 -0700 Subject: [PATCH 23/30] Changes for config variable JitDisableConstCSE - remove values 4 and 5 More fixes for GT_CNS_INT costs --- src/coreclr/src/jit/gentree.cpp | 165 +++++++++----------------- src/coreclr/src/jit/jitconfigvalues.h | 5 +- src/coreclr/src/jit/optcse.cpp | 15 +-- 3 files changed, 62 insertions(+), 123 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 683f516cca4387..12fd9741125787 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3301,8 +3301,8 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) // applied to it. // Any constant that requires a reloc must use the movw/movt sequence // - GenTreeIntConCommon* con = tree->AsIntConCommon(); - INT32 conVal = con->IconValue(); + GenTreeIntConCommon* con = tree->AsIntConCommon(); + INT32 conVal = con->IconValue(); if (con->ImmedValNeedsReloc(this)) { @@ -3313,13 +3313,13 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) else if (codeGen->validImmForInstr(INS_add, (target_ssize_t)conVal)) { // Typically included with parent oper - costSz = 1; + costSz = 2; costEx = 1; } else if (codeGen->validImmForInstr(INS_mov, (target_ssize_t)conVal) && codeGen->validImmForInstr(INS_mvn, (target_ssize_t)conVal)) { - // Uses mov ot mvn + // Uses mov or mvn costSz = 4; costEx = 1; } @@ -3397,63 +3397,6 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) } #endif // TARGET_X86 - // If JitDisableConstCSE is set to 4 or 5 we change the costs of the GT_CNS_INT - // to match that used by ARM64, so that we get a similar set of CSE performed - // when running on x64 - // - int configValue = JitConfig.JitDisableConstCSE(); - if ((configValue == 4) || (configValue == 5)) - { - GenTreeIntConCommon* con = tree->AsIntConCommon(); - bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - INT64 imm = con->LngValue(); - emitAttr size = EA_8BYTE; - - if (iconNeedsReloc) - { - costSz = 8; - costEx = 2; - } - else if ((imm >= -256) && (imm < 1024)) - { - costSz = 2; - costEx = 1; - } - else - { - // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword - // There are three forms - // movk which loads into any halfword preserving the remaining halfwords - // movz which loads into any halfword zeroing the remaining halfwords - // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting - // the register - // In some cases it is preferable to use movn, because it has the side effect of filling the - // other halfwords - // with ones - - // Determine whether movn or movz will require the fewest instructions to populate the immediate - bool preferMovz = false; - bool preferMovn = false; - int instructionCount = 4; - - for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) - { - if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) - { - preferMovz = true; // by using a movk to start we can save one instruction - instructionCount--; - } - else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) - { - preferMovn = true; // by using a movn to start we can save one instruction - instructionCount--; - } - } - - costSz = 4 * instructionCount; - costEx = instructionCount; - } - } goto COMMON_CNS; } @@ -3462,62 +3405,62 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_CNS_STR: case GT_CNS_LNG: case GT_CNS_INT: - { - GenTreeIntConCommon* con = tree->AsIntConCommon(); - bool iconNeedsReloc = con->ImmedValNeedsReloc(this); - INT64 imm = con->LngValue(); - emitAttr size = EA_8BYTE; + { + GenTreeIntConCommon* con = tree->AsIntConCommon(); + bool iconNeedsReloc = con->ImmedValNeedsReloc(this); + INT64 imm = con->LngValue(); + emitAttr size = emitActualTypeSize(tree); - if (iconNeedsReloc) - { - costSz = 8; - costEx = 2; - } - else if ((imm >= -256) && (imm < 1024)) - { - costSz = 2; - costEx = 1; - } - else if (emitter::emitIns_valid_imm_for_mov(imm, size)) - { - costSz = 4; - costEx = 1; - } - else + if (iconNeedsReloc) + { + costSz = 8; + costEx = 2; + } + else if (emitter::emitIns_valid_imm_for_add(imm, size)) + { + costSz = 2; + costEx = 1; + } + else if (emitter::emitIns_valid_imm_for_mov(imm, size)) + { + costSz = 4; + costEx = 1; + } + else + { + // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword + // There are three forms + // movk which loads into any halfword preserving the remaining halfwords + // movz which loads into any halfword zeroing the remaining halfwords + // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting + // the register + // In some cases it is preferable to use movn, because it has the side effect of filling the + // other halfwords + // with ones + + // Determine whether movn or movz will require the fewest instructions to populate the immediate + bool preferMovz = false; + bool preferMovn = false; + int instructionCount = 4; + + for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) { - // Arm64 allows any arbitrary 16-bit constant to be loaded into a register halfword - // There are three forms - // movk which loads into any halfword preserving the remaining halfwords - // movz which loads into any halfword zeroing the remaining halfwords - // movn which loads into any halfword zeroing the remaining halfwords then bitwise inverting - // the register - // In some cases it is preferable to use movn, because it has the side effect of filling the - // other halfwords - // with ones - - // Determine whether movn or movz will require the fewest instructions to populate the immediate - bool preferMovz = false; - bool preferMovn = false; - int instructionCount = 4; - - for (int i = (size == EA_8BYTE) ? 48 : 16; i >= 0; i -= 16) + if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) { - if (!preferMovn && (uint16_t(imm >> i) == 0x0000)) - { - preferMovz = true; // by using a movk to start we can save one instruction - instructionCount--; - } - else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) - { - preferMovn = true; // by using a movn to start we can save one instruction - instructionCount--; - } + preferMovz = true; // by using a movk to start we can save one instruction + instructionCount--; + } + else if (!preferMovz && (uint16_t(imm >> i) == 0xffff)) + { + preferMovn = true; // by using a movn to start we can save one instruction + instructionCount--; } - - costEx = instructionCount; - costSz = 4 * instructionCount; } + + costEx = instructionCount; + costSz = 4 * instructionCount; } + } goto COMMON_CNS; #else diff --git a/src/coreclr/src/jit/jitconfigvalues.h b/src/coreclr/src/jit/jitconfigvalues.h index 8ff5648ec3e218..6ef85008494692 100644 --- a/src/coreclr/src/jit/jitconfigvalues.h +++ b/src/coreclr/src/jit/jitconfigvalues.h @@ -289,10 +289,9 @@ CONFIG_INTEGER(JitDisableSimdVN, W("JitDisableSimdVN"), 0) // Default 0, ValueNu // If 1, disable all the CSE of Constants // If 2, enable the CSE of Constants but don't combine with nearby offsets. (only for ARM64) // If 3, enable the CSE of Constants including nearby offsets. (all platforms) -// If 4, same as 3, but also change X64 weighting of const nodes to match ARM64 costs -// If 5, same as 4, but don't combine with nearby offsets. (all platforms) +// If 4, enable the CSE of Constants but don't combine with nearby offsets. (all platforms) // -CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) +CONFIG_INTEGER(JitDisableConstCSE, W("JitDisableConstCSE"), 0) /// /// JIT diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index d4fc88cb0b4a9b..8c93c3c7ddfe19 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -410,14 +410,15 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) int configValue = JitConfig.JitDisableConstCSE(); #if defined(TARGET_ARM64) + // ARM64 - allow to combine with nearby offsets, when config is not 2 if (configValue != 2) { enableSharedConstCSE = true; } #endif // TARGET_ARM64 - // All Platforms - don't combine with nearby offsets - if (configValue == 5) + // All Platforms - also allow to combine with nearby offsets, when config is 3 + if (configValue == 3) { enableSharedConstCSE = true; } @@ -725,12 +726,14 @@ unsigned Compiler::optValnumCSE_Locate() int configValue = JitConfig.JitDisableConstCSE(); + // all platforms - disable CSE of constant values when config is 1 if (configValue == 1) { disableConstCSE = true; } #if !defined(TARGET_ARM64) - if (configValue == 0) + // non-ARM64 platforms - also disable CSE of constant values when config is 0 or 2 + if ((configValue == 0) || (configValue == 2)) { disableConstCSE = true; } @@ -2320,12 +2323,6 @@ class CSE_Heuristic // because it doesn't take into account that we might use a vector register for struct copies. slotCount = (size + TARGET_POINTER_SIZE - 1) / TARGET_POINTER_SIZE; } -#if 0 //ndef TARGET_64BIT - if (candidate->Expr()->TypeGet() == TYP_LONG) - { - slotCount = 2; // on 32-bit targets longs use two registers - } -#endif if (CodeOptKind() == Compiler::SMALL_CODE) { From 78ae0019188862cea2c3c60e9b0145ea02a362b4 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Tue, 30 Jun 2020 14:24:20 -0700 Subject: [PATCH 24/30] Allow CSE of the R2R indirect param constant address --- src/coreclr/src/jit/morph.cpp | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index e746b0b9cbd259..ac1abd5202c641 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -2707,8 +2707,6 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) indirectCellAddress->AsIntCon()->gtTargetHandle = (size_t)call->gtCallMethHnd; #endif indirectCellAddress->SetRegNum(REG_R2R_INDIRECT_PARAM); - // Don't attempt to CSE this constant - indirectCellAddress->SetDoNotCSE(); // Push the stub address onto the list of arguments. call->gtCallArgs = gtPrependNewCallArg(indirectCellAddress, call->gtCallArgs); From 34079eb7d5084c5492742c5e5ce1ddbc586f7e0b Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Tue, 30 Jun 2020 17:35:25 -0700 Subject: [PATCH 25/30] Fix Assertion failed 'isValidGeneralDatasize(size)' --- src/coreclr/src/jit/gentree.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 12fd9741125787..aa76e737207b23 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3409,7 +3409,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) GenTreeIntConCommon* con = tree->AsIntConCommon(); bool iconNeedsReloc = con->ImmedValNeedsReloc(this); INT64 imm = con->LngValue(); - emitAttr size = emitActualTypeSize(tree); + emitAttr size = EA_SIZE(emitActualTypeSize(tree)); if (iconNeedsReloc) { From baf350e30d6a441891700f62618fc7b8a17806fc Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 3 Jul 2020 11:29:51 -0700 Subject: [PATCH 26/30] Change gtCostEx for 8-byte constants on AMD64 to be 2 so that they are CSE candidates --- src/coreclr/src/jit/gentree.cpp | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index aa76e737207b23..a8baeb12da70b2 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -3337,7 +3337,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) case GT_CNS_STR: #ifdef TARGET_AMD64 costSz = 10; - costEx = 1; + costEx = 2; #else // TARGET_X86 costSz = 4; costEx = 1; @@ -3381,7 +3381,7 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree) else if (!GenTreeIntConCommon::FitsInI32(conVal)) { costSz = 10; - costEx = 1; + costEx = 2; } #endif // TARGET_AMD64 else @@ -10843,16 +10843,30 @@ void Compiler::gtDispConst(GenTree* tree) else if ((tree->AsIntCon()->gtIconVal > -1000) && (tree->AsIntCon()->gtIconVal < 1000)) { printf(" %ld", dspIconVal); -#ifdef TARGET_64BIT } +#ifdef TARGET_64BIT else if ((tree->AsIntCon()->gtIconVal & 0xFFFFFFFF00000000LL) != 0) { - printf(" 0x%llx", dspIconVal); -#endif + if (dspIconVal >= 0) + { + printf(" 0x%llx", dspIconVal); + } + else + { + printf(" -0x%llx", -dspIconVal); + } } +#endif else { - printf(" 0x%X", dspIconVal); + if (dspIconVal >= 0) + { + printf(" 0x%X", dspIconVal); + } + else + { + printf(" -0x%X", -dspIconVal); + } } if (tree->IsIconHandle()) From 0f666876c5bd2ba7196afa48a7df42c2a2029092 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 3 Jul 2020 11:31:41 -0700 Subject: [PATCH 27/30] Workaround for ARM32 assert when CSE of Consts is enabled --- src/coreclr/src/jit/morph.cpp | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index ac1abd5202c641..a6feea0d276202 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -2707,6 +2707,11 @@ void Compiler::fgInitArgInfo(GenTreeCall* call) indirectCellAddress->AsIntCon()->gtTargetHandle = (size_t)call->gtCallMethHnd; #endif indirectCellAddress->SetRegNum(REG_R2R_INDIRECT_PARAM); +#ifdef TARGET_ARM + // Don't attempt to CSE this constant + // This hits an assert: Assertion failed 'candidates != candidateBit' in lsra.cpp Line: 3723 + indirectCellAddress->SetDoNotCSE(); +#endif // TARGET_ARM // Push the stub address onto the list of arguments. call->gtCallArgs = gtPrependNewCallArg(indirectCellAddress, call->gtCallArgs); From 421fd5af405f508e61dd9666d9511eb1b742d717 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Fri, 3 Jul 2020 12:03:48 -0700 Subject: [PATCH 28/30] Add a simple sort to compute the best shared const CSE's to use for the definition, sets csdConstDefValue and csdConstDefVN Implemented shared consts CSE's with a 16-bit offset for x64, can be enabled using COMPLUS_JitDisableConstCSE=3 --- src/coreclr/src/jit/compiler.h | 15 ++- src/coreclr/src/jit/optcse.cpp | 229 +++++++++++++++++++++++++-------- 2 files changed, 191 insertions(+), 53 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 8ee88db678a684..f8398967bd37fd 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -6320,7 +6320,7 @@ class Compiler { CSEdsc* csdNextInBucket; // used by the hash table - ssize_t csdHashKey; // the orginal hashkey + size_t csdHashKey; // the orginal hashkey ssize_t csdConstDefValue; // When we CSE similar constants this is the value that we use as the def ValueNum csdConstDefVN; // When we CSE similar constants this is the ValueNumber that we use for the LclVar // assignment @@ -6356,6 +6356,19 @@ class Compiler // number, this will reflect it; otherwise, NoVN. }; +#if defined(TARGET_XARCH) +#define CSE_CONST_SHARED_LOW_BITS 16 +#else +// ARM64 or ARM32 +#define CSE_CONST_SHARED_LOW_BITS 12 +#endif + +#ifdef TARGET_64BIT +#define TARGET_SIGN_BIT (1ULL << 63) +#else +#define TARGET_SIGN_BIT (1ULL << 31) +#endif + static const size_t s_optCSEhashSize; CSEdsc** optCSEhash; CSEdsc** optCSEtab; diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index 8c93c3c7ddfe19..8d1c32a8a5cb41 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -401,7 +401,7 @@ void Compiler::optValnumCSE_Init() // unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { - ssize_t key; + size_t key; unsigned hash; unsigned hval; CSEdsc* hashDsc; @@ -463,11 +463,11 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // if (vnOp2Lib != vnLib) { - key = (ssize_t)vnLib; // include the exc set in the hash key + key = vnLib; // include the exc set in the hash key } else { - key = (ssize_t)vnLibNorm; + key = vnLibNorm; } // If we didn't do the above we would have op1 as the CSE def @@ -479,25 +479,25 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) else if (enableSharedConstCSE && tree->IsIntegralConst()) { assert(vnStore->IsVNConstant(vnLibNorm)); - key = vnStore->CoercedConstantValue(vnLibNorm); + key = vnStore->CoercedConstantValue(vnLibNorm); // We don't shared small offset constants when we require a reloc if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this)) { // Make constants that have the same upper bits use the same key - // Shift the key right by 12 bits - key = (ssize_t)(((size_t)key) >> 12); + // Shift the key right by CSE_CONST_SHARED_LOW_BITS bits and set the upper bits to zero + key >>= CSE_CONST_SHARED_LOW_BITS; } - assert(key > 0); + assert((key & TARGET_SIGN_BIT) == 0); - // We use negative values for 'key' as the flag - // that we are hashing constants (with a 12-bit offset) - key = -key; + // We use the sign bit of 'key' as the flag + // that we are hashing constants (with a shared offset) + key |= TARGET_SIGN_BIT; } else // Not a GT_COMMA or a GT_CNS_INT { - key = (ssize_t)vnLibNorm; + key = vnLibNorm; } // Compute the hash value for the expression @@ -517,7 +517,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) for (hashDsc = optCSEhash[hval]; hashDsc; hashDsc = hashDsc->csdNextInBucket) { - if (hashDsc->csdHashKey == (ssize_t)key) + if (hashDsc->csdHashKey == key) { // Check for mismatched types on GT_CNS_INT nodes if ((tree->OperGet() == GT_CNS_INT) && (tree->TypeGet() != hashDsc->csdTree->TypeGet())) @@ -629,7 +629,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) { hashDsc = new (this, CMK_CSE) CSEdsc; - hashDsc->csdHashKey = (ssize_t)key; + hashDsc->csdHashKey = key; hashDsc->csdConstDefValue = 0; hashDsc->csdConstDefVN = vnStore->VNForNull(); // uninit value hashDsc->csdIndex = 0; @@ -699,8 +699,8 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) } else { - INT64 kVal = (-key) << 12; - printf("K_%012I64x", kVal); + size_t kVal = (key & ~TARGET_SIGN_BIT) << CSE_CONST_SHARED_LOW_BITS; + printf("K_%p", dspPtr(kVal)); } printf(" in " FMT_BB ", [cost=%2u, size=%2u]: \n", compCurBB->bbNum, tree->GetCostEx(), tree->GetCostSz()); @@ -766,9 +766,12 @@ unsigned Compiler::optValnumCSE_Locate() // Don't allow CSE of constants if it is disabled // - if (disableConstCSE && tree->IsIntegralConst()) + if (tree->IsIntegralConst()) { - continue; + if (disableConstCSE) + { + continue; + } } if (!optIsCSEcandidate(tree)) @@ -1976,7 +1979,7 @@ class CSE_Heuristic cost = dsc->csdTree->GetCostEx(); } - if (dsc->csdHashKey >= 0) + if ((dsc->csdHashKey & TARGET_SIGN_BIT) == 0) { printf("CSE #%02u, {$%-3x, $%-3x} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", dsc->csdIndex, dsc->csdHashKey, dsc->defExcSetPromise, dsc->csdUseCount, def, use, cost, @@ -1984,9 +1987,9 @@ class CSE_Heuristic } else { - INT64 kVal = (-dsc->csdHashKey) >> 12; - printf("CSE #%02u, {K_%012I64x} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", - dsc->csdIndex, kVal, dsc->csdUseCount, def, use, cost, + size_t kVal = (dsc->csdHashKey & ~TARGET_SIGN_BIT) << CSE_CONST_SHARED_LOW_BITS; + printf("CSE #%02u, {K_%p} useCnt=%d: [def=%3u, use=%3u, cost=%3u%s]\n :: ", dsc->csdIndex, + dspPtr(kVal), dsc->csdUseCount, def, use, cost, dsc->csdLiveAcrossCall ? ", call" : " "); } @@ -2777,14 +2780,30 @@ class CSE_Heuristic ValueNum currVN; bool setRefCnt = true; bool allSame = true; - bool isConstCSE = (dsc->csdHashKey < 0); + bool isConstCSE = ((dsc->csdHashKey & TARGET_SIGN_BIT) != 0); + unsigned maxItem = 0; - BasicBlock::weight_t maxWeight = 0; - dsc->csdConstDefValue = -1; + struct constCSE_entry + { + ValueNum constVN; + BasicBlock::weight_t totalWeight; + ssize_t constValue; + } item[8]; + + unsigned i, j; + for (i = 0; i < 8; i++) + { + // Initialize to values that sort last + // + item[i].constVN = ValueNumStore::NoVN; + item[i].totalWeight = 0; + item[i].constValue = 0; + } lst = dsc->csdTreeList; while (lst != nullptr) { + bool isLastNode = (lst->tslNext == nullptr); // Ignore this node if the gtCSEnum value has been cleared if (IS_CSE_INDEX(lst->tslTree->gtCSEnum)) { @@ -2801,29 +2820,120 @@ class CSE_Heuristic allSame = false; } + BasicBlock* blk = lst->tslBlock; + const BasicBlock::weight_t curWeight = blk->getBBWeight(m_pCompiler); if (isConstCSE) { - BasicBlock::weight_t curWeight = lst->tslBlock->getBBWeight(m_pCompiler); - if ((curWeight > maxWeight) || (dsc->csdConstDefValue == -1)) + // See if we already have this const in our item[] table + bool match = false; + for (i = 0; i < maxItem; i++) { - maxWeight = curWeight; - dsc->csdConstDefValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); - dsc->csdConstDefVN = currVN; + if (item[i].constVN == currVN) + { + item[i].totalWeight += curWeight; + match = true; + } } - } - BasicBlock* blk = lst->tslBlock; - const BasicBlock::weight_t weight = blk->getBBWeight(m_pCompiler); + bool sortAndAdd = false; + if (match == false) + { + if (maxItem < 8) + { + j = maxItem; + // Add the new value as item[j] + // + item[j].constVN = currVN; + item[j].totalWeight = curWeight; + item[j].constValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); + + if (j == 0) + { + // Prefer to sort the very first entry as highest, so bump up its weight by 1 + item[j].totalWeight += 1; + } + maxItem++; + } + else + { + // We will sort item[*] and add this value, replacing the lowest value in item[7] + sortAndAdd = true; + } + } + + // We always need to perform one sort before finishing + // so we will sort if islastNode is true + // + if (isLastNode || sortAndAdd) + { + // sort the item[8] by totalWeight + bool firstPass = true; + bool done; + static unsigned swapTies = 0x539C6A; // 24-bits semi-randomly set + do + { + done = true; + for (i = 0; i < maxItem - 1; i++) + { + bool doSwap = (item[i].totalWeight < item[i + 1].totalWeight); + if (doSwap) + { + // We need to keep sorting as long as we perform a swap operation + done = false; + } + else + { + // In the first pass we will semi-randomly swap when the weights are the same + if (firstPass) + { + // If the weights are the same then semi-randomly chose to swap + // + if (item[i].totalWeight == item[i + 1].totalWeight) + { + // We will swap if the low bit of 'swapTies' is set + doSwap = (swapTies & 1); + if (doSwap) + { + // We will rotate 'swapTies' right by one bit + // this moves the low bit to become the upper bit + swapTies |= 0x1000000; + } + swapTies >>= 1; + } + } + } + + if (doSwap) + { + // swap item[i] and item[i+1] + struct constCSE_entry temp; + temp = item[i]; + item[i] = item[i + 1]; + item[i + 1] = temp; + } + } + firstPass = false; + } while (!done); + + if (sortAndAdd) + { + j = 7; + item[j].constVN = currVN; + item[j].totalWeight = curWeight; + item[j].constValue = m_pCompiler->vnStore->CoercedConstantValue(currVN); + } + } + } if (setRefCnt) { m_pCompiler->lvaTable[cseLclVarNum].setLvRefCnt(1); - m_pCompiler->lvaTable[cseLclVarNum].setLvRefCntWtd(weight); + m_pCompiler->lvaTable[cseLclVarNum].setLvRefCntWtd(curWeight); setRefCnt = false; } else { - m_pCompiler->lvaTable[cseLclVarNum].incRefCnts(weight, m_pCompiler); + m_pCompiler->lvaTable[cseLclVarNum].incRefCnts(curWeight, m_pCompiler); } // A CSE Def references the LclVar twice @@ -2831,30 +2941,42 @@ class CSE_Heuristic GenTree* exp = lst->tslTree; if (IS_CSE_DEF(exp->gtCSEnum)) { - m_pCompiler->lvaTable[cseLclVarNum].incRefCnts(weight, m_pCompiler); + m_pCompiler->lvaTable[cseLclVarNum].incRefCnts(curWeight, m_pCompiler); } } lst = lst->tslNext; } + dsc->csdConstDefValue = item[0].constValue; + dsc->csdConstDefVN = item[0].constVN; + #ifdef DEBUG - if (!allSame && !isConstCSE) + if (m_pCompiler->verbose) { - lst = dsc->csdTreeList; - GenTree* firstTree = lst->tslTree; - printf("In %s, CSE (oper = %s, type = %s) has differing VNs: ", m_pCompiler->info.compFullName, - GenTree::OpName(firstTree->OperGet()), varTypeName(firstTree->TypeGet())); - while (lst != nullptr) + if (isConstCSE && (maxItem > 1)) { - if (IS_CSE_INDEX(lst->tslTree->gtCSEnum)) + printf("\nWe have shared Const CSE's and selected " FMT_VN " with a value of 0x%p as the base.\n", + dsc->csdConstDefVN, dspPtr(dsc->csdConstDefValue)); + } + + if (!allSame && !isConstCSE) + { + lst = dsc->csdTreeList; + GenTree* firstTree = lst->tslTree; + printf("In %s, CSE (oper = %s, type = %s) has differing VNs: ", m_pCompiler->info.compFullName, + GenTree::OpName(firstTree->OperGet()), varTypeName(firstTree->TypeGet())); + while (lst != nullptr) { - currVN = m_pCompiler->vnStore->VNLiberalNormalValue(lst->tslTree->gtVNPair); - printf("0x%x(%s " FMT_VN ") ", lst->tslTree, IS_CSE_USE(lst->tslTree->gtCSEnum) ? "use" : "def", - currVN); + if (IS_CSE_INDEX(lst->tslTree->gtCSEnum)) + { + currVN = m_pCompiler->vnStore->VNLiberalNormalValue(lst->tslTree->gtVNPair); + printf("0x%x(%s " FMT_VN ") ", lst->tslTree, IS_CSE_USE(lst->tslTree->gtCSEnum) ? "use" : "def", + currVN); + } + lst = lst->tslNext; } - lst = lst->tslNext; + printf("\n"); } - printf("\n"); } #endif // DEBUG @@ -3244,7 +3366,7 @@ class CSE_Heuristic #ifdef DEBUG if (m_pCompiler->verbose) { - if (dsc->csdHashKey >= 0) + if ((dsc->csdHashKey & TARGET_SIGN_BIT) == 0) { printf("\nConsidering CSE #%02u {$%-3x, $%-3x} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), dsc->csdHashKey, dsc->defExcSetPromise, candidate.DefCount(), @@ -3252,9 +3374,9 @@ class CSE_Heuristic } else { - INT64 kVal = (-dsc->csdHashKey) >> 12; - printf("\nConsidering CSE #%02u {K_%012I64x} [def=%3u, use=%3u, cost=%3u%s]\n", - candidate.CseIndex(), kVal, candidate.DefCount(), candidate.UseCount(), candidate.Cost(), + size_t kVal = (dsc->csdHashKey & ~TARGET_SIGN_BIT) << CSE_CONST_SHARED_LOW_BITS; + printf("\nConsidering CSE #%02u {K_%p} [def=%3u, use=%3u, cost=%3u%s]\n", candidate.CseIndex(), + dspPtr(kVal), candidate.DefCount(), candidate.UseCount(), candidate.Cost(), dsc->csdLiveAcrossCall ? ", call" : " "); } printf("CSE Expression : \n"); @@ -3491,8 +3613,11 @@ bool Compiler::optIsCSEcandidate(GenTree* tree) return (tree->AsOp()->gtOp1->gtOper != GT_ARR_ELEM); - case GT_CNS_INT: case GT_CNS_LNG: +#ifndef TARGET_64BIT + return false; // Don't CSE 64-bit constants on 32-bit platforms +#endif + case GT_CNS_INT: case GT_CNS_DBL: case GT_CNS_STR: return true; // We reach here only when CSE_CONSTS is enabled From 83b5dc2c86d42a4c9553b6c1ef615e01511311a9 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Tue, 7 Jul 2020 12:01:58 -0700 Subject: [PATCH 29/30] Fix in AssertionProp for JitStress issue with beq_r8 test --- src/coreclr/src/jit/assertionprop.cpp | 16 ++++++++++++---- .../Old/Conformance_Base/beq_r8.ilproj | 2 +- .../Old/directed/ldarg_s_r8.ilproj | 2 +- .../JIT/Methodical/NaN/arithm64_cs_do.csproj | 2 +- .../JIT/Methodical/NaN/arithm64_cs_ro.csproj | 2 +- 5 files changed, 16 insertions(+), 8 deletions(-) diff --git a/src/coreclr/src/jit/assertionprop.cpp b/src/coreclr/src/jit/assertionprop.cpp index c50c46a115e5c8..7bac5584bf92c9 100644 --- a/src/coreclr/src/jit/assertionprop.cpp +++ b/src/coreclr/src/jit/assertionprop.cpp @@ -3236,7 +3236,8 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen return nullptr; } - AssertionDsc* curAssertion = optGetAssertion(index); + AssertionDsc* curAssertion = optGetAssertion(index); + bool assertionKindIsEqual = (curAssertion->assertionKind == OAK_EQUAL); // Allow or not to reverse condition for OAK_NOT_EQUAL assertions. bool allowReverse = true; @@ -3251,7 +3252,7 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen printf("\nVN relop based constant assertion prop in " FMT_BB ":\n", compCurBB->bbNum); printf("Assertion index=#%02u: ", index); printTreeID(op1); - printf(" %s ", (curAssertion->assertionKind == OAK_EQUAL) ? "==" : "!="); + printf(" %s ", assertionKindIsEqual ? "==" : "!="); if (genActualType(op1->TypeGet()) == TYP_INT) { printf("%d\n", vnStore->ConstantValue(vnCns)); @@ -3336,8 +3337,15 @@ GenTree* Compiler::optAssertionPropGlobal_RelOp(ASSERT_VALARG_TP assertions, Gen op1->gtVNPair.SetBoth(vnCns); // Preserve the ValueNumPair, as ChangeOperConst/SetOper will clear it. - // Also set the value number on the relop. - if (curAssertion->assertionKind == OAK_EQUAL) + // set foldResult to either 0 or 1 + bool foldResult = assertionKindIsEqual; + if (tree->gtOper == GT_NE) + { + foldResult = !foldResult; + } + + // Set the value number on the relop to 1 (true) or 0 (false) + if (foldResult) { tree->gtVNPair.SetBoth(vnStore->VNOneForType(TYP_INT)); } diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r8.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r8.ilproj index e92e57de25c758..ca0982693df230 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r8.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r8.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj b/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj index c9963628da1229..51c64145355e68 100644 --- a/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/Methodical/NaN/arithm64_cs_do.csproj b/src/tests/JIT/Methodical/NaN/arithm64_cs_do.csproj index 645b85538bf65c..63895dbcfb49da 100644 --- a/src/tests/JIT/Methodical/NaN/arithm64_cs_do.csproj +++ b/src/tests/JIT/Methodical/NaN/arithm64_cs_do.csproj @@ -1,7 +1,7 @@ Exe - 1 + 0 Full diff --git a/src/tests/JIT/Methodical/NaN/arithm64_cs_ro.csproj b/src/tests/JIT/Methodical/NaN/arithm64_cs_ro.csproj index a453a807abb145..21d4a502b6b400 100644 --- a/src/tests/JIT/Methodical/NaN/arithm64_cs_ro.csproj +++ b/src/tests/JIT/Methodical/NaN/arithm64_cs_ro.csproj @@ -1,7 +1,7 @@ Exe - 1 + 0 None From 435bdd579b3e337359c2dac553aa5ce8ff8ec3d1 Mon Sep 17 00:00:00 2001 From: Brian Sullivan Date: Wed, 8 Jul 2020 10:00:25 -0700 Subject: [PATCH 30/30] Added additional Priority 0 test coverage for Floating Point optimizations Coverage for Floating point CSEs, conditional branch AssertionProp and NaN handling --- src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r4.ilproj | 2 +- src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r4.ilproj | 2 +- src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r8.ilproj | 2 +- .../JIT/IL_Conformance/Old/Conformance_Base/bge_un_r4.ilproj | 2 +- .../JIT/IL_Conformance/Old/Conformance_Base/bge_un_r8.ilproj | 2 +- .../JIT/IL_Conformance/Old/Conformance_Base/bne_un_r4.ilproj | 2 +- .../JIT/IL_Conformance/Old/Conformance_Base/bne_un_r8.ilproj | 2 +- src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj | 2 +- src/tests/JIT/Methodical/NaN/arithm64_cs_d.csproj | 2 +- src/tests/JIT/Methodical/NaN/arithm64_cs_r.csproj | 2 +- 10 files changed, 10 insertions(+), 10 deletions(-) diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r4.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r4.ilproj index cef275ea49426b..ab208980834058 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r4.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/beq_r4.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r4.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r4.ilproj index b5e9a76cdac4e9..191bb09523af66 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r4.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r4.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r8.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r8.ilproj index 62ed8ad09721d2..5e8c0aed782cfa 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r8.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_r8.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r4.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r4.ilproj index ba5b4cd8572028..b0a46391cdbd70 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r4.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r4.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r8.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r8.ilproj index ca67fefcb7644e..8d689526c4de68 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r8.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bge_un_r8.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r4.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r4.ilproj index 891402dd071ab3..a3b92f67f70bc8 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r4.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r4.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r8.ilproj b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r8.ilproj index eb0db5833a1da5..ededde0d346d15 100644 --- a/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r8.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/Conformance_Base/bne_un_r8.ilproj @@ -2,7 +2,7 @@ Exe true - 1 + 0 PdbOnly diff --git a/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj b/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj index 51c64145355e68..c9963628da1229 100644 --- a/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj +++ b/src/tests/JIT/IL_Conformance/Old/directed/ldarg_s_r8.ilproj @@ -2,7 +2,7 @@ Exe true - 0 + 1 PdbOnly diff --git a/src/tests/JIT/Methodical/NaN/arithm64_cs_d.csproj b/src/tests/JIT/Methodical/NaN/arithm64_cs_d.csproj index 3c8a6b09632fab..882dbd5ce4de96 100644 --- a/src/tests/JIT/Methodical/NaN/arithm64_cs_d.csproj +++ b/src/tests/JIT/Methodical/NaN/arithm64_cs_d.csproj @@ -1,7 +1,7 @@ Exe - 1 + 0 Full diff --git a/src/tests/JIT/Methodical/NaN/arithm64_cs_r.csproj b/src/tests/JIT/Methodical/NaN/arithm64_cs_r.csproj index f70b11917026e4..750ade69062123 100644 --- a/src/tests/JIT/Methodical/NaN/arithm64_cs_r.csproj +++ b/src/tests/JIT/Methodical/NaN/arithm64_cs_r.csproj @@ -1,7 +1,7 @@ Exe - 1 + 0 None