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 5 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
179 changes: 74 additions & 105 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,17 @@ 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)
GenTree* op = impPopStack().val;
if ((op->gtFlags & GTF_SIDE_EFFECT) != 0)
{
JITDUMP("\n Importing BOX; BR_TRUE/FALSE as %sconstant\n",
treeToNullcheck == nullptr ? "" : "nullcheck+");
impPopStack();

GenTree* result = gtNewIconNode(1);

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

impPushOnStack(result, typeInfo(TYP_INT));
return 0;
impStoreTemp(lvaGrabTemp(true DEBUGARG("spill side effects")), op, CHECK_SPILL_ALL);
}
Copy link
Member

Choose a reason for hiding this comment

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

This is a bit of an unusual way of doing this in the importer, use impSpillSideEffect instead?

Also we already do this from the caller for BoxPatterns::IsByRefLike. Should it be unified?

Copy link
Member Author

@EgorBo EgorBo Aug 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Ah, thanks for the hint. addressed.

Also we already do this from the caller for BoxPatterns::IsByRefLike. Should it be unified?

Looks like that one has to be always done regardless wether we match a pattern or not while here we only do this when we match it. Presumably the byref case is rare so it's ok to call it twice for simplicity?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like that one has to be always done regardless wether we match a pattern or not while here we only do this when we match it.

I don't think so (note that the comment is misleading -- we BADCODE today on unmatched patterns, we do not generate code to throw InvalidProgramException at runtime). But I'm ok with leaving it to avoid further risk and potentially cleaning it up in .NET 9, assuming you want this in .NET 8.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, I see, yeah let's clean it up in .NET 9.0, it has a potential to slightly refactor the whole thing

JITDUMP("\n Importing BOX; BR_TRUE/FALSE as constant\n")
impPushOnStack(gtNewTrue(), typeInfo(TYP_INT));
return 0;
}
}
break;
Expand All @@ -2926,7 +2889,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,7 +2898,12 @@ int Compiler::impBoxPatternMatch(CORINFO_RESOLVED_TOKEN* pResolvedToken,
TypeCompareState::MustNot)
{
JITDUMP("\n Importing BOX; ISINST; as null\n");
impPopStack();

GenTree* op = impPopStack().val;
if ((op->gtFlags & GTF_SIDE_EFFECT) != 0)
{
impStoreTemp(lvaGrabTemp(true DEBUGARG("spill side effects")), op, CHECK_SPILL_ALL);
}
impPushOnStack(gtNewNull(), typeInfo(TYP_REF));
return 1 + sizeof(mdToken);
}
Expand All @@ -2958,78 +2926,79 @@ 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);
GenTree* op = impPopStack().val;
if ((op->gtFlags & GTF_SIDE_EFFECT) != 0)
{
impStoreTemp(lvaGrabTemp(true DEBUGARG("spill side effects")), op,
CHECK_SPILL_ALL);
}
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