Skip to content

Remove boxing from "value is S" for nullables #95711

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Dec 7, 2023
Merged

Remove boxing from "value is S" for nullables #95711

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
f0c4fff
Proper fix
EgorBo Dec 7, 2023
4375aad
Clean up
EgorBo Dec 7, 2023
b1b2831
Update importer.cpp
EgorBo Dec 7, 2023
ea1b014
Update importer.cpp
EgorBo Dec 7, 2023
b2af515
reject isbyreflike
EgorBo Dec 7, 2023
7053273
Merge branch 'main' of github.com:dotnet/runtime into box-isinst-null…
EgorBo Dec 7, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
197 changes: 110 additions & 87 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2953,128 +2953,151 @@ 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
//
Comment on lines +2965 to +2974
Copy link
Member

@huoyaoyuan huoyaoyuan Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did similar transform in #47920 (comment) . It's interesting to see which implementation covers more cases.

I'll push it later today.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I tried this transformation I while back and gave up due to small diffs, decided to push it because of customer reported issue

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

main...huoyaoyuan:runtime:isinst-null-cmp
It doesn't help, and doesn't conflict either. Let's do separately.

Copy link
Member Author

@EgorBo EgorBo Dec 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think my change is simpler? (if you ignore whitespaces).

Also, not sure if box + isinst + ldnull + ceq ever shows up - Roslyn seems to not emitting it so I didn't bother

E.g. sharplab


// 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 ((opts == BoxPatterns::IsByRefLike) ||
(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);
}
static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0);
impPushOnStack(gtNewIndir(TYP_UBYTE, objToBox), typeInfo(TYP_INT));

JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as nullableVT.hasValue\n");
return returnToken;
}
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 returnToken;
}
}
break;
}
break;

// box + isinst + unbox.any
case CEE_UNBOX_ANY:
if ((nextCodeAddr + 1 + sizeof(mdToken)) <= codeEndp)
{
if (opts == BoxPatterns::MakeInlineObservation)
{
if (opts == BoxPatterns::MakeInlineObservation)
{
compInlineResult->Note(InlineObservation::CALLEE_FOLDABLE_BOX);
return 2 + sizeof(mdToken) * 2;
}
compInlineResult->Note(InlineObservation::CALLEE_FOLDABLE_BOX);
return 2 + sizeof(mdToken) * 2;
}

// 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);
// 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,
if (info.compCompHnd->compareTypesForEquality(isinstResolvedToken.hClass,
pResolvedToken->hClass) == TypeCompareState::Must)
{
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)
{
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;
}
JITDUMP("\n Importing BOX; ISINST, UNBOX.ANY as NOP\n");
return 2 + sizeof(mdToken) * 2;
}
}
}
break;

default:
break;
}
}
Expand Down