From ad090d9acc45b807dafc066c7df0d7ef97200434 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sun, 22 Jan 2023 21:19:44 +0100 Subject: [PATCH 1/5] 1) Don't add implicit cast for icon handles (nint -> byref) 2) Cache FieldSeq* for addresses --- src/coreclr/jit/valuenum.cpp | 63 +++++++++++++++++++++++++++--------- src/coreclr/jit/valuenum.h | 18 +++++++++++ 2 files changed, 65 insertions(+), 16 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 16380ac8d1911b..d390ea358e218f 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -436,6 +436,7 @@ ValueNumStore::ValueNumStore(Compiler* comp, CompAllocator alloc) , m_longCnsMap(nullptr) , m_handleMap(nullptr) , m_embeddedToCompileTimeHandleMap(alloc) + , m_fieldAddressToFieldSeqMap(alloc) , m_floatCnsMap(nullptr) , m_doubleCnsMap(nullptr) , m_byrefCnsMap(nullptr) @@ -8253,12 +8254,18 @@ void Compiler::fgValueNumberTreeConst(GenTree* tree) case TYP_BOOL: if (tree->IsIconHandle()) { - const ssize_t embeddedHandle = tree->AsIntCon()->IconValue(); - tree->gtVNPair.SetBoth(vnStore->VNForHandle(embeddedHandle, tree->GetIconHandleFlag())); - if (tree->GetIconHandleFlag() == GTF_ICON_CLASS_HDL) + const GenTreeIntCon* cns = tree->AsIntCon(); + const GenTreeFlags handleFlags = tree->GetIconHandleFlag(); + tree->gtVNPair.SetBoth(vnStore->VNForHandle(cns->IconValue(), handleFlags)); + if (handleFlags == GTF_ICON_CLASS_HDL) { - const ssize_t compileTimeHandle = tree->AsIntCon()->gtCompileTimeHandle; - vnStore->AddToEmbeddedHandleMap(embeddedHandle, compileTimeHandle); + vnStore->AddToEmbeddedHandleMap(cns->IconValue(), cns->gtCompileTimeHandle); + } + else if ((handleFlags == GTF_ICON_STATIC_HDL) && (cns->gtFieldSeq != nullptr) && + (cns->gtFieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStaticKnownAddress)) + { + // For now we're interested only in SimpleStaticKnownAddress + vnStore->AddToFieldAddressToFieldSeqMap(cns->IconValue(), cns->gtFieldSeq); } } else if ((typ == TYP_LONG) || (typ == TYP_ULONG)) @@ -8569,13 +8576,13 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s ValueNum op1vn = op1->gtVNPair.GetLiberal(); ValueNum op2vn = op2->gtVNPair.GetLiberal(); - if (!op1->IsIconHandle(GTF_ICON_STATIC_HDL) && op1->gtVNPair.BothEqual() && vnStore->IsVNConstant(op1vn) && + if (op1->gtVNPair.BothEqual() && vnStore->IsVNConstant(op1vn) && !vnStore->IsVNHandle(op1vn) && varTypeIsIntegral(vnStore->TypeOfVN(op1vn))) { val += vnStore->CoercedConstantValue(op1vn); tree = op2; } - else if (!op2->IsIconHandle(GTF_ICON_STATIC_HDL) && op2->gtVNPair.BothEqual() && vnStore->IsVNConstant(op2vn) && + else if (op2->gtVNPair.BothEqual() && vnStore->IsVNConstant(op2vn) && !vnStore->IsVNHandle(op2vn) && varTypeIsIntegral(vnStore->TypeOfVN(op2vn))) { val += vnStore->CoercedConstantValue(op2vn); @@ -8589,12 +8596,18 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s } // Base address is expected to be static field's address - if ((tree->IsCnsIntOrI()) && (tree->AsIntCon()->gtFieldSeq != nullptr) && - (tree->AsIntCon()->gtFieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStaticKnownAddress)) + ValueNum treeVN = tree->gtVNPair.GetLiberal(); + if (tree->gtVNPair.BothEqual() && vnStore->IsVNConstant(treeVN) && vnStore->IsVNHandle(treeVN) && + (vnStore->GetHandleFlags(treeVN) == GTF_ICON_STATIC_HDL)) { - *pFseq = tree->AsIntCon()->gtFieldSeq; - *byteOffset = tree->AsIntCon()->IconValue() + val - tree->AsIntCon()->gtFieldSeq->GetOffset(); - return true; + ssize_t icon = vnStore->CoercedConstantValue(treeVN); + FieldSeq* fldSeq = vnStore->GetFieldSeqFromAddress(icon); + if (fldSeq != nullptr) + { + *pFseq = fldSeq; + *byteOffset = icon + val - fldSeq->GetOffset(); + return true; + } } return false; } @@ -9807,9 +9820,19 @@ ValueNumPair ValueNumStore::VNPairForCast(ValueNumPair srcVNPair, bool srcIsUnsigned, /* = false */ bool hasOverflowCheck) /* = false */ { - ValueNum srcLibVN = srcVNPair.GetLiberal(); - ValueNum srcConVN = srcVNPair.GetConservative(); - ValueNum castLibVN = VNForCast(srcLibVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); + ValueNum srcLibVN = srcVNPair.GetLiberal(); + ValueNum srcConVN = srcVNPair.GetConservative(); + + ValueNum castLibVN; + if ((castFromType == TYP_I_IMPL) && (castToType == TYP_BYREF) && IsVNHandle(srcLibVN)) + { + // Omit cast for (h)CNS_INT [TYP_I_IMPL -> TYP_BYREF] + castLibVN = srcLibVN; + } + else + { + castLibVN = VNForCast(srcLibVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); + } ValueNum castConVN; if (srcVNPair.BothEqual()) @@ -9818,7 +9841,15 @@ ValueNumPair ValueNumStore::VNPairForCast(ValueNumPair srcVNPair, } else { - castConVN = VNForCast(srcConVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); + if ((castFromType == TYP_I_IMPL) && (castToType == TYP_BYREF) && IsVNHandle(srcConVN)) + { + // Omit cast for (h)CNS_INT [TYP_I_IMPL -> TYP_BYREF] + castConVN = srcConVN; + } + else + { + castConVN = VNForCast(srcConVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); + } } return {castLibVN, castConVN}; diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index c949243c08060a..2af14b1d9d2953 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -463,6 +463,21 @@ class ValueNumStore m_embeddedToCompileTimeHandleMap.AddOrUpdate(embeddedHandle, compileTimeHandle); } + void AddToFieldAddressToFieldSeqMap(ssize_t fldAddr, FieldSeq* fldSeq) + { + m_fieldAddressToFieldSeqMap.AddOrUpdate(fldAddr, fldSeq); + } + + FieldSeq* GetFieldSeqFromAddress(ssize_t fldAddr) + { + FieldSeq* fldSeq; + if (m_fieldAddressToFieldSeqMap.TryGetValue(fldAddr, &fldSeq)) + { + return fldSeq; + } + return nullptr; + } + // And the single constant for an object reference type. static ValueNum VNForNull() { @@ -1423,6 +1438,9 @@ class ValueNumStore typedef SmallHashTable EmbeddedToCompileTimeHandleMap; EmbeddedToCompileTimeHandleMap m_embeddedToCompileTimeHandleMap; + typedef SmallHashTable FieldAddressToFieldSeqMap; + FieldAddressToFieldSeqMap m_fieldAddressToFieldSeqMap; + struct LargePrimitiveKeyFuncsFloat : public JitLargePrimitiveKeyFuncs { static bool Equals(float x, float y) From 44c18ebece4323659db9e5c235ab8c1fe9747651 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 23 Jan 2023 02:04:10 +0100 Subject: [PATCH 2/5] Fix size regressions --- src/coreclr/jit/assertionprop.cpp | 33 ++++++++++++++++++------------- src/coreclr/jit/compiler.h | 2 +- 2 files changed, 20 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 648a01774dbbed..a7f46e2b770802 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3227,13 +3227,10 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree) if (conValTree != nullptr) { - if (tree->OperIs(GT_LCL_VAR)) + if (!optIsProfitableToSubstitute(tree, block, conValTree)) { - if (!optIsProfitableToSubstitute(tree->AsLclVar(), block, conValTree)) - { - // Not profitable to substitute - return nullptr; - } + // Not profitable to substitute + return nullptr; } // Were able to optimize. @@ -3259,27 +3256,35 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree) } //------------------------------------------------------------------------------ -// optIsProfitableToSubstitute: Checks if value worth substituting to lcl location +// optIsProfitableToSubstitute: Checks if value worth substituting to dest // // Arguments: -// lcl - lcl to replace with value if profitable -// lclBlock - Basic block lcl located in -// value - value we plan to substitute to lcl +// dest - destination to substitute value to +// destBlock - Basic block of destination +// value - value we plan to substitute // // Returns: // False if it's likely not profitable to do substitution, True otherwise // -bool Compiler::optIsProfitableToSubstitute(GenTreeLclVarCommon* lcl, BasicBlock* lclBlock, GenTree* value) +bool Compiler::optIsProfitableToSubstitute(GenTree* dest, BasicBlock* destBlock, GenTree* value) { + // Giving up on these kinds of handles demonstrated size improvements + if (!value->IsIntegralConst(0) && value->IsIconHandle(GTF_ICON_STATIC_HDL, GTF_ICON_CLASS_HDL)) + { + return false; + } + // A simple heuristic: If the constant is defined outside of a loop (not far from its head) // and is used inside it - don't propagate. // TODO: Extend on more kinds of trees - if (!value->OperIs(GT_CNS_VEC, GT_CNS_DBL)) + if (!value->OperIs(GT_CNS_VEC, GT_CNS_DBL) || !dest->IsLocal()) { return true; } + const GenTreeLclVarCommon* lcl = dest->AsLclVarCommon(); + gtPrepareCost(value); if ((value->GetCostEx() > 1) && (value->GetCostSz() > 1)) @@ -3294,11 +3299,11 @@ bool Compiler::optIsProfitableToSubstitute(GenTreeLclVarCommon* lcl, BasicBlock* // NOTE: this currently does not take "a float living across a call" case into account // where we might end up with spill/restore on ABIs without callee-saved registers const weight_t defBlockWeight = defBlock->getBBWeight(this); - const weight_t lclblockWeight = lclBlock->getBBWeight(this); + const weight_t lclblockWeight = destBlock->getBBWeight(this); if ((defBlockWeight > 0) && ((lclblockWeight / defBlockWeight) >= BB_LOOP_WEIGHT_SCALE)) { - JITDUMP("Constant propagation inside loop " FMT_BB " is not profitable\n", lclBlock->bbNum); + JITDUMP("Constant propagation inside loop " FMT_BB " is not profitable\n", destBlock->bbNum); return false; } } diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 5b451505a6ec2a..1e239bdc2ed55a 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -7471,7 +7471,7 @@ class Compiler GenTree* optConstantAssertionProp(AssertionDsc* curAssertion, GenTreeLclVarCommon* tree, Statement* stmt DEBUGARG(AssertionIndex index)); - bool optIsProfitableToSubstitute(GenTreeLclVarCommon* lcl, BasicBlock* lclBlock, GenTree* value); + bool optIsProfitableToSubstitute(GenTree* dest, BasicBlock* destBlock, GenTree* value); bool optZeroObjAssertionProp(GenTree* tree, ASSERT_VALARG_TP assertions); // Assertion propagation functions. From 9f088d2b2435340369492ada8265fb15440043ad Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 23 Jan 2023 17:10:46 +0100 Subject: [PATCH 3/5] Address feedback --- src/coreclr/jit/assertionprop.cpp | 4 +-- src/coreclr/jit/valuenum.cpp | 47 +++++++++++++------------------ src/coreclr/jit/valuenum.h | 6 ++-- 3 files changed, 25 insertions(+), 32 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index a7f46e2b770802..87d4b6eddfbb35 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3278,12 +3278,12 @@ bool Compiler::optIsProfitableToSubstitute(GenTree* dest, BasicBlock* destBlock, // and is used inside it - don't propagate. // TODO: Extend on more kinds of trees - if (!value->OperIs(GT_CNS_VEC, GT_CNS_DBL) || !dest->IsLocal()) + if (!value->OperIs(GT_CNS_VEC, GT_CNS_DBL) || !dest->OperIs(GT_LCL_VAR)) { return true; } - const GenTreeLclVarCommon* lcl = dest->AsLclVarCommon(); + const GenTreeLclVar* lcl = dest->AsLclVar(); gtPrepareCost(value); diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index d390ea358e218f..c46704932b796b 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8264,8 +8264,10 @@ void Compiler::fgValueNumberTreeConst(GenTree* tree) else if ((handleFlags == GTF_ICON_STATIC_HDL) && (cns->gtFieldSeq != nullptr) && (cns->gtFieldSeq->GetKind() == FieldSeq::FieldKind::SimpleStaticKnownAddress)) { + assert(cns->IconValue() == cns->gtFieldSeq->GetOffset()); + // For now we're interested only in SimpleStaticKnownAddress - vnStore->AddToFieldAddressToFieldSeqMap(cns->IconValue(), cns->gtFieldSeq); + vnStore->AddToFieldAddressToFieldSeqMap(cns->gtVNPair.GetLiberal(), cns->gtFieldSeq); } } else if ((typ == TYP_LONG) || (typ == TYP_ULONG)) @@ -8576,13 +8578,12 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s ValueNum op1vn = op1->gtVNPair.GetLiberal(); ValueNum op2vn = op2->gtVNPair.GetLiberal(); - if (op1->gtVNPair.BothEqual() && vnStore->IsVNConstant(op1vn) && !vnStore->IsVNHandle(op1vn) && - varTypeIsIntegral(vnStore->TypeOfVN(op1vn))) + if (op1->gtVNPair.BothEqual() && !vnStore->IsVNHandle(op1vn) && varTypeIsIntegral(vnStore->TypeOfVN(op1vn))) { val += vnStore->CoercedConstantValue(op1vn); tree = op2; } - else if (op2->gtVNPair.BothEqual() && vnStore->IsVNConstant(op2vn) && !vnStore->IsVNHandle(op2vn) && + else if (op2->gtVNPair.BothEqual() && !vnStore->IsVNHandle(op2vn) && varTypeIsIntegral(vnStore->TypeOfVN(op2vn))) { val += vnStore->CoercedConstantValue(op2vn); @@ -8597,15 +8598,17 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s // Base address is expected to be static field's address ValueNum treeVN = tree->gtVNPair.GetLiberal(); - if (tree->gtVNPair.BothEqual() && vnStore->IsVNConstant(treeVN) && vnStore->IsVNHandle(treeVN) && + if (tree->gtVNPair.BothEqual() && vnStore->IsVNHandle(treeVN) && (vnStore->GetHandleFlags(treeVN) == GTF_ICON_STATIC_HDL)) { - ssize_t icon = vnStore->CoercedConstantValue(treeVN); - FieldSeq* fldSeq = vnStore->GetFieldSeqFromAddress(icon); + FieldSeq* fldSeq = vnStore->GetFieldSeqFromAddress(treeVN); if (fldSeq != nullptr) { + assert(fldSeq->GetKind() == FieldSeq::FieldKind::SimpleStaticKnownAddress); + assert(fldSeq->GetOffset() == vnStore->CoercedConstantValue(treeVN)); + *pFseq = fldSeq; - *byteOffset = icon + val - fldSeq->GetOffset(); + *byteOffset = val; return true; } } @@ -9781,6 +9784,13 @@ ValueNum ValueNumStore::VNForCast(ValueNum srcVN, bool srcIsUnsigned, /* = false */ bool hasOverflowCheck) /* = false */ { + + if ((castFromType == TYP_I_IMPL) && (castToType == TYP_BYREF) && IsVNHandle(srcVN)) + { + // Omit cast for (h)CNS_INT [TYP_I_IMPL -> TYP_BYREF] + return srcVN; + } + // The resulting type after performing the cast is always widened to a supported IL stack size var_types resultType = genActualType(castToType); @@ -9823,16 +9833,7 @@ ValueNumPair ValueNumStore::VNPairForCast(ValueNumPair srcVNPair, ValueNum srcLibVN = srcVNPair.GetLiberal(); ValueNum srcConVN = srcVNPair.GetConservative(); - ValueNum castLibVN; - if ((castFromType == TYP_I_IMPL) && (castToType == TYP_BYREF) && IsVNHandle(srcLibVN)) - { - // Omit cast for (h)CNS_INT [TYP_I_IMPL -> TYP_BYREF] - castLibVN = srcLibVN; - } - else - { - castLibVN = VNForCast(srcLibVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); - } + ValueNum castLibVN = VNForCast(srcLibVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); ValueNum castConVN; if (srcVNPair.BothEqual()) @@ -9841,15 +9842,7 @@ ValueNumPair ValueNumStore::VNPairForCast(ValueNumPair srcVNPair, } else { - if ((castFromType == TYP_I_IMPL) && (castToType == TYP_BYREF) && IsVNHandle(srcConVN)) - { - // Omit cast for (h)CNS_INT [TYP_I_IMPL -> TYP_BYREF] - castConVN = srcConVN; - } - else - { - castConVN = VNForCast(srcConVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); - } + castConVN = VNForCast(srcConVN, castToType, castFromType, srcIsUnsigned, hasOverflowCheck); } return {castLibVN, castConVN}; diff --git a/src/coreclr/jit/valuenum.h b/src/coreclr/jit/valuenum.h index 2af14b1d9d2953..f08d6baa35c502 100644 --- a/src/coreclr/jit/valuenum.h +++ b/src/coreclr/jit/valuenum.h @@ -463,12 +463,12 @@ class ValueNumStore m_embeddedToCompileTimeHandleMap.AddOrUpdate(embeddedHandle, compileTimeHandle); } - void AddToFieldAddressToFieldSeqMap(ssize_t fldAddr, FieldSeq* fldSeq) + void AddToFieldAddressToFieldSeqMap(ValueNum fldAddr, FieldSeq* fldSeq) { m_fieldAddressToFieldSeqMap.AddOrUpdate(fldAddr, fldSeq); } - FieldSeq* GetFieldSeqFromAddress(ssize_t fldAddr) + FieldSeq* GetFieldSeqFromAddress(ValueNum fldAddr) { FieldSeq* fldSeq; if (m_fieldAddressToFieldSeqMap.TryGetValue(fldAddr, &fldSeq)) @@ -1438,7 +1438,7 @@ class ValueNumStore typedef SmallHashTable EmbeddedToCompileTimeHandleMap; EmbeddedToCompileTimeHandleMap m_embeddedToCompileTimeHandleMap; - typedef SmallHashTable FieldAddressToFieldSeqMap; + typedef SmallHashTable FieldAddressToFieldSeqMap; FieldAddressToFieldSeqMap m_fieldAddressToFieldSeqMap; struct LargePrimitiveKeyFuncsFloat : public JitLargePrimitiveKeyFuncs From ddc3105f24999239bffeea69c6bbabfc54e9fd94 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Mon, 23 Jan 2023 17:13:47 +0100 Subject: [PATCH 4/5] oops removed too much --- src/coreclr/jit/valuenum.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index c46704932b796b..869cdca7a8bec3 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8578,12 +8578,13 @@ static bool GetStaticFieldSeqAndAddress(ValueNumStore* vnStore, GenTree* tree, s ValueNum op1vn = op1->gtVNPair.GetLiberal(); ValueNum op2vn = op2->gtVNPair.GetLiberal(); - if (op1->gtVNPair.BothEqual() && !vnStore->IsVNHandle(op1vn) && varTypeIsIntegral(vnStore->TypeOfVN(op1vn))) + if (op1->gtVNPair.BothEqual() && vnStore->IsVNConstant(op1vn) && !vnStore->IsVNHandle(op1vn) && + varTypeIsIntegral(vnStore->TypeOfVN(op1vn))) { val += vnStore->CoercedConstantValue(op1vn); tree = op2; } - else if (op2->gtVNPair.BothEqual() && !vnStore->IsVNHandle(op2vn) && + else if (op2->gtVNPair.BothEqual() && vnStore->IsVNConstant(op2vn) && !vnStore->IsVNHandle(op2vn) && varTypeIsIntegral(vnStore->TypeOfVN(op2vn))) { val += vnStore->CoercedConstantValue(op2vn); From 82a6132d669fc6cfc723e4fc2f936929cf33804b Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Tue, 24 Jan 2023 13:56:41 +0100 Subject: [PATCH 5/5] Update src/coreclr/jit/assertionprop.cpp Co-authored-by: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> --- src/coreclr/jit/assertionprop.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 87d4b6eddfbb35..a45e8be2d41a51 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3269,7 +3269,7 @@ GenTree* Compiler::optVNConstantPropOnTree(BasicBlock* block, GenTree* tree) bool Compiler::optIsProfitableToSubstitute(GenTree* dest, BasicBlock* destBlock, GenTree* value) { // Giving up on these kinds of handles demonstrated size improvements - if (!value->IsIntegralConst(0) && value->IsIconHandle(GTF_ICON_STATIC_HDL, GTF_ICON_CLASS_HDL)) + if (value->IsIconHandle(GTF_ICON_STATIC_HDL, GTF_ICON_CLASS_HDL)) { return false; }