From 5d7112c321448cbf89f06bc4fa770407108bac0d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 04:30:42 +0200 Subject: [PATCH 01/14] don't propagate const TYP_REF as TYP_I_IMPL --- src/coreclr/jit/assertionprop.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index b140c8caf8aa8..c12516dfe5c43 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3420,6 +3420,9 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, // Here we have to allocate a new 'large' node to replace the old one newTree = gtNewIconHandleNode(curAssertion->op2.u1.iconVal, curAssertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK); + + // Make sure we don't change TYP_REF to an integer + newTree->ChangeType(tree->TypeGet()); } else { From a22fecc30e46aca83458edea8f976d267675ec83 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 04:47:37 +0200 Subject: [PATCH 02/14] Fix more asserts --- src/coreclr/jit/emitxarch.cpp | 2 +- src/coreclr/jit/gentree.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index 20a700e691340..bb66729e06ab1 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -13080,7 +13080,7 @@ BYTE* emitter::emitOutputRI(BYTE* dst, instrDesc* id) break; case IF_RRW_CNS: - assert(id->idGCref() == GCT_BYREF); + assert((id->idGCref() == GCT_BYREF) || (id->idGCref() == GCT_GCREF)); #ifdef DEBUG regMaskTP regMask; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 9913e6f2ce4d7..5e59d8e0272e5 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -11121,7 +11121,7 @@ void Compiler::gtDispConst(GenTree* tree) if (tree->TypeGet() == TYP_REF) { - assert(tree->AsIntCon()->gtIconVal == 0); + assert((tree->AsIntCon()->gtIconVal == 0) || doesMethodHaveFrozenString()); printf(" null"); } else if ((tree->AsIntCon()->gtIconVal > -1000) && (tree->AsIntCon()->gtIconVal < 1000)) From bfdd32b327ada77d5626c307b2532cd540a79047 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 04:51:09 +0200 Subject: [PATCH 03/14] fix dump --- src/coreclr/jit/gentree.cpp | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 5e59d8e0272e5..ccff92a320ff3 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -11121,8 +11121,15 @@ void Compiler::gtDispConst(GenTree* tree) if (tree->TypeGet() == TYP_REF) { - assert((tree->AsIntCon()->gtIconVal == 0) || doesMethodHaveFrozenString()); - printf(" null"); + if (tree->AsIntCon()->gtIconVal == 0) + { + printf(" null"); + } + else + { + assert(doesMethodHaveFrozenString()); + printf(" 0x%llx", dspIconVal); + } } else if ((tree->AsIntCon()->gtIconVal > -1000) && (tree->AsIntCon()->gtIconVal < 1000)) { From e152c0c50ec0277ed424e00c7e0f54134d54faad Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 14:40:28 +0200 Subject: [PATCH 04/14] Allow re-typing for TYP_REF null (to minimize spmi diffs) --- src/coreclr/jit/assertionprop.cpp | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index c12516dfe5c43..e5513c9552198 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3421,8 +3421,11 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, newTree = gtNewIconHandleNode(curAssertion->op2.u1.iconVal, curAssertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK); - // Make sure we don't change TYP_REF to an integer - newTree->ChangeType(tree->TypeGet()); + // Make sure we don't change TYP_REF to an integer for non-null handles + if (!tree->IsIntegralConst(0)) + { + newTree->ChangeType(TYP_REF); + } } else { From ece6a3dd3b6cc4bcf6bb7d79c59756cb4662de86 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 15:30:08 +0200 Subject: [PATCH 05/14] Don't "share" const gc handles --- src/coreclr/jit/emitxarch.cpp | 2 +- src/coreclr/jit/optcse.cpp | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/emitxarch.cpp b/src/coreclr/jit/emitxarch.cpp index bb66729e06ab1..20a700e691340 100644 --- a/src/coreclr/jit/emitxarch.cpp +++ b/src/coreclr/jit/emitxarch.cpp @@ -13080,7 +13080,7 @@ BYTE* emitter::emitOutputRI(BYTE* dst, instrDesc* id) break; case IF_RRW_CNS: - assert((id->idGCref() == GCT_BYREF) || (id->idGCref() == GCT_GCREF)); + assert(id->idGCref() == GCT_BYREF); #ifdef DEBUG regMaskTP regMask; diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index 76912c0e174ac..a2d1d90bf5fb4 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -478,8 +478,9 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) assert(vnStore->IsVNConstant(vnLibNorm)); // We don't share small offset constants when they require a reloc + // Also, we don't share non-null const gc handles // - if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this)) + if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && ((tree->IsIntegralConst(0)) || !tree->TypeIs(TYP_REF))) { // Here we make constants that have the same upper bits use the same key // From fcebb53e2916d61d19f7f795683b927501b0f94e Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 15:40:30 +0200 Subject: [PATCH 06/14] Address feedback --- src/coreclr/jit/optcse.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/optcse.cpp b/src/coreclr/jit/optcse.cpp index a2d1d90bf5fb4..3d300d898e8bd 100644 --- a/src/coreclr/jit/optcse.cpp +++ b/src/coreclr/jit/optcse.cpp @@ -480,7 +480,7 @@ unsigned Compiler::optValnumCSE_Index(GenTree* tree, Statement* stmt) // We don't share small offset constants when they require a reloc // Also, we don't share non-null const gc handles // - if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && ((tree->IsIntegralConst(0)) || !tree->TypeIs(TYP_REF))) + if (!tree->AsIntConCommon()->ImmedValNeedsReloc(this) && ((tree->IsIntegralConst(0)) || !varTypeIsGC(tree))) { // Here we make constants that have the same upper bits use the same key // From 04b7f02d26b6a73a6944325260af7642a635f6cb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 17:01:31 +0200 Subject: [PATCH 07/14] VN replace STR_HDL with CONST_HDL --- src/coreclr/jit/valuenum.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 9b6238101fab2..0761a0106a941 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8162,7 +8162,7 @@ void Compiler::fgValueNumberTreeConst(GenTree* tree) } else { - assert(tree->IsIconHandle(GTF_ICON_STR_HDL)); // Constant object can be only frozen string. + assert(doesMethodHaveFrozenString()); // Constant object can be only frozen string. tree->gtVNPair.SetBoth( vnStore->VNForHandle(ssize_t(tree->AsIntConCommon()->IconValue()), tree->GetIconHandleFlag())); } From 52609d121655a1c82816015a745be479b47431b9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 15 Sep 2022 19:17:03 +0200 Subject: [PATCH 08/14] Looks like we used to allow const prop for TYP_REF <- TYP_I_IMPL for non-zero constants, e.g. GTF_ICON_STATIC_HDL --- src/coreclr/jit/assertionprop.cpp | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index e5513c9552198..a5d2561dd8597 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3421,8 +3421,9 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, newTree = gtNewIconHandleNode(curAssertion->op2.u1.iconVal, curAssertion->op2.u1.iconFlags & GTF_ICON_HDL_MASK); - // Make sure we don't change TYP_REF to an integer for non-null handles - if (!tree->IsIntegralConst(0)) + // Make sure we don't retype const gc handles to TYP_I_IMPL + // Although, it's possible for e.g. GTF_ICON_STATIC_HDL + if (!tree->IsIntegralConst(0) && tree->IsIconHandle(GTF_ICON_STR_HDL)) { newTree->ChangeType(TYP_REF); } From 2f89efa7dbe861705f8d26a8170a852ce3545547 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 16 Sep 2022 00:52:16 +0200 Subject: [PATCH 09/14] Fix unfortunate typo :( --- 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 a5d2561dd8597..55b5e3501d4c7 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3423,7 +3423,7 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, // Make sure we don't retype const gc handles to TYP_I_IMPL // Although, it's possible for e.g. GTF_ICON_STATIC_HDL - if (!tree->IsIntegralConst(0) && tree->IsIconHandle(GTF_ICON_STR_HDL)) + if (!newTree->IsIntegralConst(0) && newTree->IsIconHandle(GTF_ICON_STR_HDL)) { newTree->ChangeType(TYP_REF); } From 2785b2d560055b25d9f12204f5f712eeb247767d Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 16 Sep 2022 16:14:16 +0200 Subject: [PATCH 10/14] fix ref2byref_il_r test --- src/coreclr/jit/assertionprop.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 55b5e3501d4c7..d83f1aa4fdb4a 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3425,7 +3425,8 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, // Although, it's possible for e.g. GTF_ICON_STATIC_HDL if (!newTree->IsIntegralConst(0) && newTree->IsIconHandle(GTF_ICON_STR_HDL)) { - newTree->ChangeType(TYP_REF); + assert(varTypeIsGC(tree)); + newTree->ChangeType(tree->TypeGet()); } } else From 1cae571a5c941a1cafe275e95d9c2dc82ab3897b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 16 Sep 2022 16:15:09 +0200 Subject: [PATCH 11/14] fix critical_finalization test --- .../baseservices/critical_finalization/critical_finalization.cs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/tests/baseservices/critical_finalization/critical_finalization.cs b/src/tests/baseservices/critical_finalization/critical_finalization.cs index 49c2de2b37fab..553ccfc5a1b95 100644 --- a/src/tests/baseservices/critical_finalization/critical_finalization.cs +++ b/src/tests/baseservices/critical_finalization/critical_finalization.cs @@ -48,6 +48,6 @@ class T : P { return 1; if (Q.first_p_count < P.count) return 1; - return 0; + return 100; } } From 20829d3cd016122d37eb04753e7b38f399af8388 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Fri, 16 Sep 2022 16:29:28 +0200 Subject: [PATCH 12/14] disallow REF -> BYREF propagation --- src/coreclr/jit/assertionprop.cpp | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index d83f1aa4fdb4a..016a2976811ce 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3425,8 +3425,13 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, // Although, it's possible for e.g. GTF_ICON_STATIC_HDL if (!newTree->IsIntegralConst(0) && newTree->IsIconHandle(GTF_ICON_STR_HDL)) { - assert(varTypeIsGC(tree)); - newTree->ChangeType(tree->TypeGet()); + if (!tree->TypeIs(TYP_REF)) + { + assert(tree->TypeIs(TYP_BYREF)); + // Conservatively don't allow propagation of ICON TYP_REF into BYREF + return nullptr; + } + newTree->ChangeType(TYP_REF); } } else From 9421d59ea57f137e1f916f00421b9763c2225778 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 16 Sep 2022 20:06:34 +0200 Subject: [PATCH 13/14] assert was wrong - in case of pinned locals it's fine to pass int handles --- src/coreclr/jit/assertionprop.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/assertionprop.cpp b/src/coreclr/jit/assertionprop.cpp index 016a2976811ce..a6ba864388557 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3425,13 +3425,12 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, // Although, it's possible for e.g. GTF_ICON_STATIC_HDL if (!newTree->IsIntegralConst(0) && newTree->IsIconHandle(GTF_ICON_STR_HDL)) { - if (!tree->TypeIs(TYP_REF)) + if (tree->TypeIs(TYP_BYREF)) { - assert(tree->TypeIs(TYP_BYREF)); // Conservatively don't allow propagation of ICON TYP_REF into BYREF return nullptr; } - newTree->ChangeType(TYP_REF); + newTree->ChangeType(newTree->TypeGet()); } } else From a9d6fa7f778d9349f7f99229e5518bb782217876 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Fri, 16 Sep 2022 20:07:35 +0200 Subject: [PATCH 14/14] Update assertionprop.cpp --- 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 a6ba864388557..430287284e6f1 100644 --- a/src/coreclr/jit/assertionprop.cpp +++ b/src/coreclr/jit/assertionprop.cpp @@ -3430,7 +3430,7 @@ GenTree* Compiler::optConstantAssertionProp(AssertionDsc* curAssertion, // Conservatively don't allow propagation of ICON TYP_REF into BYREF return nullptr; } - newTree->ChangeType(newTree->TypeGet()); + newTree->ChangeType(tree->TypeGet()); } } else