From f0c4fff7b2c7bb370bb9579fcaceac7cd97d64d6 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 7 Dec 2023 03:52:08 +0100 Subject: [PATCH 1/5] Proper fix --- src/coreclr/jit/importer.cpp | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index a6ddc2a8cb6e9f..6201298b6613fe 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3077,6 +3077,40 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, } } break; + + // box + isinst + ldnull + cgt.un + case CEE_LDNULL: + if ((impGetNonPrefixOpcode(nextCodeAddr + 1, codeEndp) == CEE_CGT_UN) && + (opts == BoxPatterns::None) && + (info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX_NULLABLE) && + ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0)) + { + // The code is copied from "BOX + ISINST + BR_TRUE/FALSE" case above. + + CORINFO_RESOLVED_TOKEN isInstTok; + impResolveToken(codeAddr + 1, &isInstTok, CORINFO_TOKENKIND_Casting); + + CORINFO_CLASS_HANDLE underlyingCls = + info.compCompHnd->getTypeForBox(pResolvedToken->hClass); + TypeCompareState castResult = + info.compCompHnd->compareTypesForCast(underlyingCls, isInstTok.hClass); + + if (castResult == TypeCompareState::Must) + { + GenTree* objToBox = impGetNodeAddr(impPopStack().val, CHECK_SPILL_ALL, nullptr); + impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox), typeInfo(TYP_INT)); + JITDUMP("\n Importing BOX; ISINST; LDNULL; CGT_UN as nullableVT.hasValue\n") + return 4 + sizeof(mdToken); + } + if (castResult == TypeCompareState::MustNot) + { + impPopStack(); + impPushOnStack(gtNewFalse(), typeInfo(TYP_INT)); + JITDUMP("\n Importing BOX; ISINST; LDNULL; CGT_UN as constant (false)\n") + return 4 + sizeof(mdToken); + } + } + break; } } break; From 4375aadd078dcf58f316f3c6d2c800c3cec003fb Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 7 Dec 2023 04:12:39 +0100 Subject: [PATCH 2/5] Clean up --- src/coreclr/jit/importer.cpp | 229 ++++++++++++++++------------------- 1 file changed, 107 insertions(+), 122 deletions(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 6201298b6613fe..b4f88aded650a3 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -2954,163 +2954,148 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, } } - const BYTE* nextCodeAddr = codeAddr + 1 + sizeof(mdToken); - - switch (nextCodeAddr[0]) + const BYTE* nextCodeAddr = codeAddr + 1 + sizeof(mdToken); + const OPCODE nextOpcode = impGetNonPrefixOpcode(nextCodeAddr, codeEndp); + switch (nextOpcode) { // box + isinst + br_true/false case CEE_BRTRUE: case CEE_BRTRUE_S: case CEE_BRFALSE: case CEE_BRFALSE_S: - if ((nextCodeAddr + ((nextCodeAddr[0] >= CEE_BRFALSE) ? 5 : 2)) <= codeEndp) + case CEE_LDNULL: + { + // "ldnull + cgt_un" is used when BOX+ISINST is not fed into a branch, e.g.: + // + // if (obj is string) { <--- box + isinst + br_true + // + // bool b = obj is string; <--- box + isinst + ldnull + cgt_un + // + // bool b = obj is not string; <--- box + isinst + ldnull + cgt_un + ldc.i4.0 + ceq + // + + // For br_true/false, we'll only replace "box + isinst" to a boolean + int returnToken = 1 + sizeof(mdToken); + if ((nextOpcode == CEE_LDNULL)) { - if (opts == BoxPatterns::MakeInlineObservation) + // for ldnull case, we'll replace the whole "box + isinst + ldnull + cgt_un" sequence + returnToken = 4 + sizeof(mdToken); + if (impGetNonPrefixOpcode(nextCodeAddr + 1, codeEndp) != CEE_CGT_UN) { - compInlineResult->Note(InlineObservation::CALLEE_FOLDABLE_BOX); - return 1 + sizeof(mdToken); + break; } + } - CorInfoHelpFunc foldAsHelper; - if (opts == BoxPatterns::IsByRefLike) - { - // Treat ByRefLike types as if they were regular boxing operations - // so they can be elided. - foldAsHelper = CORINFO_HELP_BOX; - } - else - { - foldAsHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); - } + if (opts == BoxPatterns::MakeInlineObservation) + { + compInlineResult->Note(InlineObservation::CALLEE_FOLDABLE_BOX); + return returnToken; + } - if (foldAsHelper == CORINFO_HELP_BOX) - { - CORINFO_RESOLVED_TOKEN isInstResolvedToken; + CorInfoHelpFunc foldAsHelper; + if (opts == BoxPatterns::IsByRefLike) + { + // Treat ByRefLike types as if they were regular boxing operations + // so they can be elided. + foldAsHelper = CORINFO_HELP_BOX; + } + else + { + foldAsHelper = info.compCompHnd->getBoxHelper(pResolvedToken->hClass); + } - impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting); + if (foldAsHelper == CORINFO_HELP_BOX) + { + CORINFO_RESOLVED_TOKEN isInstResolvedToken; - TypeCompareState castResult = - info.compCompHnd->compareTypesForCast(pResolvedToken->hClass, - isInstResolvedToken.hClass); + impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting); - if (castResult != TypeCompareState::May) - { - JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as constant\n"); + TypeCompareState castResult = + info.compCompHnd->compareTypesForCast(pResolvedToken->hClass, + isInstResolvedToken.hClass); - impSpillSideEffects(false, CHECK_SPILL_ALL DEBUGARG("spilling side-effects")); - impPopStack(); - impPushOnStack(gtNewIconNode((castResult == TypeCompareState::Must) ? 1 : 0), - typeInfo(TYP_INT)); - return 1 + sizeof(mdToken); - } - } - else if ((foldAsHelper == CORINFO_HELP_BOX_NULLABLE) && - ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0)) + if (castResult != TypeCompareState::May) { - // For nullable we're going to fold it to "ldfld hasValue + brtrue/brfalse" or - // "ldc.i4.0 + brtrue/brfalse" in case if the underlying type is not castable to - // the target type. - CORINFO_RESOLVED_TOKEN isInstResolvedToken; - impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting); + JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as constant\n"); - CORINFO_CLASS_HANDLE nullableCls = pResolvedToken->hClass; - CORINFO_CLASS_HANDLE underlyingCls = info.compCompHnd->getTypeForBox(nullableCls); + impSpillSideEffects(false, CHECK_SPILL_ALL DEBUGARG("spilling side-effects")); + impPopStack(); + impPushOnStack(gtNewIconNode((castResult == TypeCompareState::Must) ? 1 : 0), + typeInfo(TYP_INT)); + return returnToken; + } + } + else if ((foldAsHelper == CORINFO_HELP_BOX_NULLABLE) && + ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0)) + { + // For nullable we're going to fold it to "ldfld hasValue + brtrue/brfalse" or + // "ldc.i4.0 + brtrue/brfalse" in case if the underlying type is not castable to + // the target type. + CORINFO_RESOLVED_TOKEN isInstResolvedToken; + impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting); - TypeCompareState castResult = - info.compCompHnd->compareTypesForCast(underlyingCls, isInstResolvedToken.hClass); + CORINFO_CLASS_HANDLE nullableCls = pResolvedToken->hClass; + CORINFO_CLASS_HANDLE underlyingCls = info.compCompHnd->getTypeForBox(nullableCls); - if (castResult == TypeCompareState::Must) - { - GenTree* objToBox = impPopStack().val; + TypeCompareState castResult = + info.compCompHnd->compareTypesForCast(underlyingCls, isInstResolvedToken.hClass); - // Spill struct to get its address (to access hasValue field) - // TODO-Bug?: verify if flags matter here - GenTreeFlags indirFlags = GTF_EMPTY; - objToBox = impGetNodeAddr(objToBox, CHECK_SPILL_ALL, &indirFlags); + if (castResult == TypeCompareState::Must) + { + GenTree* objToBox = impPopStack().val; - static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); - impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox), typeInfo(TYP_INT)); + // Spill struct to get its address (to access hasValue field) + // TODO-Bug?: verify if flags matter here + GenTreeFlags indirFlags = GTF_EMPTY; + objToBox = impGetNodeAddr(objToBox, CHECK_SPILL_ALL, &indirFlags); - JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as nullableVT.hasValue\n"); - return 1 + sizeof(mdToken); - } - else if (castResult == TypeCompareState::MustNot) - { - impPopStack(); - impPushOnStack(gtNewIconNode(0), typeInfo(TYP_INT)); - JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as constant (false)\n"); - return 1 + sizeof(mdToken); - } - } - } - break; + static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0); + impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox), typeInfo(TYP_INT)); - // box + isinst + unbox.any - case CEE_UNBOX_ANY: - if ((nextCodeAddr + 1 + sizeof(mdToken)) <= codeEndp) - { - if (opts == BoxPatterns::MakeInlineObservation) - { - compInlineResult->Note(InlineObservation::CALLEE_FOLDABLE_BOX); - return 2 + sizeof(mdToken) * 2; + JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as nullableVT.hasValue\n"); + return returnToken; } - - // See if the resolved tokens in box, isinst and unbox.any describe types that are equal. - CORINFO_RESOLVED_TOKEN isinstResolvedToken = {}; - impResolveToken(codeAddr + 1, &isinstResolvedToken, CORINFO_TOKENKIND_Class); - - if (info.compCompHnd->compareTypesForEquality(isinstResolvedToken.hClass, - pResolvedToken->hClass) == - TypeCompareState::Must) + else if (castResult == TypeCompareState::MustNot) { - CORINFO_RESOLVED_TOKEN unboxResolvedToken = {}; - impResolveToken(nextCodeAddr + 1, &unboxResolvedToken, CORINFO_TOKENKIND_Class); - - // If so, box + isinst + unbox.any is a nop. - if (info.compCompHnd->compareTypesForEquality(unboxResolvedToken.hClass, - pResolvedToken->hClass) == - TypeCompareState::Must) - { - JITDUMP("\n Importing BOX; ISINST, UNBOX.ANY as NOP\n"); - return 2 + sizeof(mdToken) * 2; - } + impPopStack(); + impPushOnStack(gtNewIconNode(0), typeInfo(TYP_INT)); + JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as constant (false)\n"); + return returnToken; } } - break; + } + break; - // box + isinst + ldnull + cgt.un - case CEE_LDNULL: - if ((impGetNonPrefixOpcode(nextCodeAddr + 1, codeEndp) == CEE_CGT_UN) && - (opts == BoxPatterns::None) && - (info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX_NULLABLE) && - ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0)) + // box + isinst + unbox.any + case CEE_UNBOX_ANY: + { + if (opts == BoxPatterns::MakeInlineObservation) { - // The code is copied from "BOX + ISINST + BR_TRUE/FALSE" case above. + compInlineResult->Note(InlineObservation::CALLEE_FOLDABLE_BOX); + return 2 + sizeof(mdToken) * 2; + } - CORINFO_RESOLVED_TOKEN isInstTok; - impResolveToken(codeAddr + 1, &isInstTok, CORINFO_TOKENKIND_Casting); + // See if the resolved tokens in box, isinst and unbox.any describe types that are equal. + CORINFO_RESOLVED_TOKEN isinstResolvedToken = {}; + impResolveToken(codeAddr + 1, &isinstResolvedToken, CORINFO_TOKENKIND_Class); - CORINFO_CLASS_HANDLE underlyingCls = - info.compCompHnd->getTypeForBox(pResolvedToken->hClass); - TypeCompareState castResult = - info.compCompHnd->compareTypesForCast(underlyingCls, isInstTok.hClass); + if (info.compCompHnd->compareTypesForEquality(isinstResolvedToken.hClass, + pResolvedToken->hClass) == TypeCompareState::Must) + { + CORINFO_RESOLVED_TOKEN unboxResolvedToken = {}; + impResolveToken(nextCodeAddr + 1, &unboxResolvedToken, CORINFO_TOKENKIND_Class); - if (castResult == TypeCompareState::Must) - { - GenTree* objToBox = impGetNodeAddr(impPopStack().val, CHECK_SPILL_ALL, nullptr); - impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox), typeInfo(TYP_INT)); - JITDUMP("\n Importing BOX; ISINST; LDNULL; CGT_UN as nullableVT.hasValue\n") - return 4 + sizeof(mdToken); - } - if (castResult == TypeCompareState::MustNot) + // If so, box + isinst + unbox.any is a nop. + if (info.compCompHnd->compareTypesForEquality(unboxResolvedToken.hClass, + pResolvedToken->hClass) == + TypeCompareState::Must) { - impPopStack(); - impPushOnStack(gtNewFalse(), typeInfo(TYP_INT)); - JITDUMP("\n Importing BOX; ISINST; LDNULL; CGT_UN as constant (false)\n") - return 4 + sizeof(mdToken); + JITDUMP("\n Importing BOX; ISINST, UNBOX.ANY as NOP\n"); + return 2 + sizeof(mdToken) * 2; } } - break; + } + break; } } break; From b1b283166df7b310564e07ad3eb7051a0512be57 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 7 Dec 2023 04:26:08 +0100 Subject: [PATCH 3/5] Update importer.cpp --- src/coreclr/jit/importer.cpp | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index b4f88aded650a3..26ff5022312ff6 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -3096,6 +3096,9 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, } } break; + + default: + break; } } break; From ea1b014c40e3639b1b11b3cabb19680d1c900eb6 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 7 Dec 2023 04:29:01 +0100 Subject: [PATCH 4/5] Update importer.cpp --- src/coreclr/jit/importer.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 26ff5022312ff6..1c2944c9f255ab 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -2976,7 +2976,7 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, // For br_true/false, we'll only replace "box + isinst" to a boolean int returnToken = 1 + sizeof(mdToken); - if ((nextOpcode == CEE_LDNULL)) + if (nextOpcode == CEE_LDNULL) { // for ldnull case, we'll replace the whole "box + isinst + ldnull + cgt_un" sequence returnToken = 4 + sizeof(mdToken); From b2af51571e1ce87db3e3d4e8d260aa00f5e3632b Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 7 Dec 2023 11:22:27 +0100 Subject: [PATCH 5/5] reject isbyreflike --- src/coreclr/jit/importer.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index 1c2944c9f255ab..8cf613ffe621aa 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -2980,7 +2980,8 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken, { // for ldnull case, we'll replace the whole "box + isinst + ldnull + cgt_un" sequence returnToken = 4 + sizeof(mdToken); - if (impGetNonPrefixOpcode(nextCodeAddr + 1, codeEndp) != CEE_CGT_UN) + if ((opts == BoxPatterns::IsByRefLike) || + (impGetNonPrefixOpcode(nextCodeAddr + 1, codeEndp) != CEE_CGT_UN)) { break; }