Skip to content

Handle side-effects in impBoxPatternMatch #90496

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 8 commits into from
Aug 14, 2023
Merged

Handle side-effects in impBoxPatternMatch #90496

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
9613a71
Be less conservative in impBoxPatternMatch
EgorBo Aug 13, 2023
f791c02
Fix asserts
EgorBo Aug 14, 2023
168e6ec
Actually, it's no longer needed
EgorBo Aug 14, 2023
587eb27
clean up
EgorBo Aug 14, 2023
9eefa4e
Update importer.cpp
EgorBo Aug 14, 2023
9335402
Address feedback
EgorBo Aug 14, 2023
313cef2
wrong one
EgorBo Aug 14, 2023
4372334
less conservative
EgorBo Aug 14, 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
172 changes: 66 additions & 106 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2869,54 +2869,15 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken,
return 0;
}

GenTree* const treeToBox = impStackTop().val;
bool canOptimize = true;
GenTree* treeToNullcheck = nullptr;

// Can the thing being boxed cause a side effect?
if ((treeToBox->gtFlags & GTF_SIDE_EFFECT) != 0)
{
// Is this a side effect we can replicate cheaply?
if (((treeToBox->gtFlags & GTF_SIDE_EFFECT) == GTF_EXCEPT) && treeToBox->OperIs(GT_BLK, GT_IND))
{
// If the only side effect comes from the dereference itself, yes.
GenTree* const addr = treeToBox->AsOp()->gtGetOp1();

if ((addr->gtFlags & GTF_SIDE_EFFECT) != 0)
{
canOptimize = false;
}
else if (fgAddrCouldBeNull(addr))
{
treeToNullcheck = addr;
}
}
else
{
canOptimize = false;
}
}

if (canOptimize)
if ((opts == BoxPatterns::IsByRefLike) ||
(info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX))
{
if ((opts == BoxPatterns::IsByRefLike) ||
info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX)
{
JITDUMP("\n Importing BOX; BR_TRUE/FALSE as %sconstant\n",
treeToNullcheck == nullptr ? "" : "nullcheck+");
impPopStack();

GenTree* result = gtNewIconNode(1);
JITDUMP("\n Importing BOX; BR_TRUE/FALSE as constant\n")

if (treeToNullcheck != nullptr)
{
GenTree* nullcheck = gtNewNullCheck(treeToNullcheck, compCurBB);
result = gtNewOperNode(GT_COMMA, TYP_INT, nullcheck, result);
}

impPushOnStack(result, typeInfo(TYP_INT));
return 0;
}
impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("spilling side-effects"));
Copy link
Member

Choose a reason for hiding this comment

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

Did you check diffs for this? I'm not sure we want to spill all side effects, it should be enough to spill the top one (and the slots it interferes with). I also don't think we need to pass true. Not sure if there is any significant difference given the side-effecting trees we would expect at the top of the stack here, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Diffs - I've also checked that all side-effects are preserved in my tests

impPopStack();
impPushOnStack(gtNewTrue(), typeInfo(TYP_INT));
return 0;
}
}
break;
Expand All @@ -2926,7 +2887,7 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken,
{
// First, let's see if we can fold BOX+ISINST to just null if ISINST is known to return null
// for the given argument. Don't make inline observations for this case.
if ((opts == BoxPatterns::None) && ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0) &&
if ((opts == BoxPatterns::None) &&
(info.compCompHnd->getBoxHelper(pResolvedToken->hClass) == CORINFO_HELP_BOX))
{
CORINFO_RESOLVED_TOKEN isInstTok;
Expand All @@ -2935,6 +2896,8 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken,
TypeCompareState::MustNot)
{
JITDUMP("\n Importing BOX; ISINST; as null\n");

impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("spilling side-effects"));
impPopStack();
impPushOnStack(gtNewNull(), typeInfo(TYP_REF));
return 1 + sizeof(mdToken);
Expand All @@ -2958,78 +2921,75 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken,
return 1 + sizeof(mdToken);
}

if ((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0)
CorInfoHelpFunc foldAsHelper;
if (opts == BoxPatterns::IsByRefLike)
{
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);
}
// 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 (foldAsHelper == CORINFO_HELP_BOX)
{
CORINFO_RESOLVED_TOKEN isInstResolvedToken;
if (foldAsHelper == CORINFO_HELP_BOX)
{
CORINFO_RESOLVED_TOKEN isInstResolvedToken;

impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting);
impResolveToken(codeAddr + 1, &isInstResolvedToken, CORINFO_TOKENKIND_Casting);

TypeCompareState castResult =
info.compCompHnd->compareTypesForCast(pResolvedToken->hClass,
isInstResolvedToken.hClass);
if (castResult != TypeCompareState::May)
{
JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as constant\n");
impPopStack();
TypeCompareState castResult =
info.compCompHnd->compareTypesForCast(pResolvedToken->hClass,
isInstResolvedToken.hClass);

impPushOnStack(gtNewIconNode((castResult == TypeCompareState::Must) ? 1 : 0),
typeInfo(TYP_INT));
if (castResult != TypeCompareState::May)
{
JITDUMP("\n Importing BOX; ISINST; BR_TRUE/FALSE as constant\n");

// Skip the next isinst instruction
return 1 + sizeof(mdToken);
}
impSpillSideEffects(true, 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)
{
// 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);
}
else if ((foldAsHelper == CORINFO_HELP_BOX_NULLABLE) &&
((impStackTop().val->gtFlags & GTF_SIDE_EFFECT) == 0))
Copy link
Member Author

Choose a reason for hiding this comment

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

Didn't do it for nullable due to empty diffs for it (would have to add tests as the logic is a bit different).

{
// 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);

CORINFO_CLASS_HANDLE nullableCls = pResolvedToken->hClass;
CORINFO_CLASS_HANDLE underlyingCls = info.compCompHnd->getTypeForBox(nullableCls);
CORINFO_CLASS_HANDLE nullableCls = pResolvedToken->hClass;
CORINFO_CLASS_HANDLE underlyingCls = info.compCompHnd->getTypeForBox(nullableCls);

TypeCompareState castResult =
info.compCompHnd->compareTypesForCast(underlyingCls,
isInstResolvedToken.hClass);
TypeCompareState castResult =
info.compCompHnd->compareTypesForCast(underlyingCls, isInstResolvedToken.hClass);

if (castResult == TypeCompareState::Must)
{
GenTree* objToBox = impPopStack().val;
if (castResult == TypeCompareState::Must)
{
GenTree* objToBox = impPopStack().val;

// 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);
// 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);

static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0);
impPushOnStack(gtNewIndir(TYP_BOOL, objToBox), typeInfo(TYP_INT));
static_assert_no_msg(OFFSETOF__CORINFO_NullableOfT__hasValue == 0);
impPushOnStack(gtNewIndir(TYP_BOOL, objToBox), typeInfo(TYP_INT));

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);
}
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);
}
}
}
Expand Down
1 change: 0 additions & 1 deletion ...ies/System.Private.CoreLib/src/System/Runtime/CompilerServices/AsyncTaskMethodBuilderT.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,7 +93,6 @@ internal static void AwaitUnsafeOnCompleted<TAwaiter, TStateMachine>(
AwaitUnsafeOnCompleted(ref awaiter, box);
}

[MethodImpl(MethodImplOptions.AggressiveOptimization)] // workaround boxing allocations in Tier0: https://github.com/dotnet/runtime/issues/9120
internal static void AwaitUnsafeOnCompleted<TAwaiter>(
ref TAwaiter awaiter, IAsyncStateMachineBox box)
where TAwaiter : ICriticalNotifyCompletion
Expand Down