Skip to content
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

R2R fixes for [Equality]Comparer.Default in jit #75422

Closed
wants to merge 2 commits into from
Closed

R2R fixes for [Equality]Comparer.Default in jit #75422

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
7e07ab9
R2R fixes for [Equlity]Compare.get_Default in jit
EgorBo Sep 11, 2022
da2fc47
Address feedback
EgorBo Sep 14, 2022
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
2 changes: 2 additions & 0 deletions src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4631,6 +4631,8 @@ class Compiler

GenTreeCall* fgGetStaticsCCtorHelper(CORINFO_CLASS_HANDLE cls, CorInfoHelpFunc helper);

bool fgIsCurrentCallDceCandidate();

GenTreeCall* fgGetSharedCCtor(CORINFO_CLASS_HANDLE cls);

bool backendRequiresLocalVarLifetimes()
Expand Down
32 changes: 4 additions & 28 deletions src/coreclr/jit/fginline.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1630,17 +1630,14 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)

// For case (1)
//
// Look for the following tree shapes
// prejit: (IND (ADD (CONST, CALL(special dce helper...))))
// jit : (COMMA (CALL(special dce helper...), (FIELD ...)))
if (argNode->gtOper == GT_COMMA)
if (argNode->OperIs(GT_COMMA))
{
// Look for (COMMA (CALL(special dce helper...), (FIELD ...)))
GenTree* op1 = argNode->AsOp()->gtOp1;
GenTree* op2 = argNode->AsOp()->gtOp2;
GenTree* op1 = argNode->gtGetOp1();
GenTree* op2 = argNode->gtGetOp2();
if (op1->IsCall() &&
((op1->AsCall()->gtCallMoreFlags & GTF_CALL_M_HELPER_SPECIAL_DCE) != 0) &&
(op2->gtOper == GT_FIELD) && ((op2->gtFlags & GTF_EXCEPT) == 0))
(op2->OperIs(GT_FIELD)) && ((op2->gtFlags & GTF_EXCEPT) == 0))
{
JITDUMP("\nPerforming special dce on unused arg [%06u]:"
" actual arg [%06u] helper call [%06u]\n",
Expand All @@ -1649,27 +1646,6 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
append = false;
}
}
else if (argNode->gtOper == GT_IND)
{
// Look for (IND (ADD (CONST, CALL(special dce helper...))))
GenTree* addr = argNode->AsOp()->gtOp1;

if (addr->gtOper == GT_ADD)
{
GenTree* op1 = addr->AsOp()->gtOp1;
GenTree* op2 = addr->AsOp()->gtOp2;
if (op1->IsCall() &&
((op1->AsCall()->gtCallMoreFlags & GTF_CALL_M_HELPER_SPECIAL_DCE) != 0) &&
op2->IsCnsIntOrI())
{
// Drop the whole tree
JITDUMP("\nPerforming special dce on unused arg [%06u]:"
" actual arg [%06u] helper call [%06u]\n",
argNode->gtTreeID, argNode->gtTreeID, op1->gtTreeID);
append = false;
}
}
}
}

if (!append)
Expand Down
39 changes: 28 additions & 11 deletions src/coreclr/jit/flowgraph.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -793,6 +793,31 @@ GenTreeLclVar* Compiler::fgIsIndirOfAddrOfLocal(GenTree* tree)
return res;
}

//------------------------------------------------------------------------
// fgIsCurrentCallDceCandidate: Determine whether the currently compiled method
// is a DCE candidate
//
// Returns:
// True if the currently compiled method can be removed if it's not used
// despite potential side-effects
//
bool Compiler::fgIsCurrentCallDceCandidate()
{
// If we're importing the special EqualityComparer<T>.Default or Comparer<T>.Default
// intrinsics, flag the helper call. Later during inlining, we can
// remove the helper call if the associated field lookup is unused.
if ((info.compFlags & CORINFO_FLG_INTRINSIC) != 0)
{
NamedIntrinsic ni = lookupNamedIntrinsic(info.compMethodHnd);
if ((ni == NI_System_Collections_Generic_EqualityComparer_get_Default) ||
(ni == NI_System_Collections_Generic_Comparer_get_Default))
{
return true;
}
}
return false;
}

GenTreeCall* Compiler::fgGetStaticsCCtorHelper(CORINFO_CLASS_HANDLE cls, CorInfoHelpFunc helper)
{
bool bNeedClassID = true;
Expand Down Expand Up @@ -891,18 +916,10 @@ GenTreeCall* Compiler::fgGetStaticsCCtorHelper(CORINFO_CLASS_HANDLE cls, CorInfo

result->gtFlags |= callFlags;

// If we're importing the special EqualityComparer<T>.Default or Comparer<T>.Default
// intrinsics, flag the helper call. Later during inlining, we can
// remove the helper call if the associated field lookup is unused.
if ((info.compFlags & CORINFO_FLG_INTRINSIC) != 0)
if (fgIsCurrentCallDceCandidate())
{
NamedIntrinsic ni = lookupNamedIntrinsic(info.compMethodHnd);
if ((ni == NI_System_Collections_Generic_EqualityComparer_get_Default) ||
(ni == NI_System_Collections_Generic_Comparer_get_Default))
{
JITDUMP("\nmarking helper call [%06u] as special dce...\n", result->gtTreeID);
result->gtCallMoreFlags |= GTF_CALL_M_HELPER_SPECIAL_DCE;
}
JITDUMP("\nmarking helper call [%06u] as special dce...\n", result->gtTreeID);
result->gtCallMoreFlags |= GTF_CALL_M_HELPER_SPECIAL_DCE;
}

return result;
Expand Down
27 changes: 26 additions & 1 deletion src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9035,6 +9035,12 @@ GenTree* Compiler::impImportStaticFieldAccess(CORINFO_RESOLVED_TOKEN* pResolvedT
op1->gtFlags |= callFlags;

op1->AsCall()->setEntryPoint(pFieldInfo->fieldLookup);

if (fgIsCurrentCallDceCandidate())
{
JITDUMP("\nmarking helper call [%06u] as special dce...\n", op1->gtTreeID);
op1->AsCall()->gtCallMoreFlags |= GTF_CALL_M_HELPER_SPECIAL_DCE;
}
}
else
#endif
Expand Down Expand Up @@ -9761,9 +9767,28 @@ var_types Compiler::impImportCall(OPCODE opcode,
{
// Null check is sometimes needed for ready to run to handle
// non-virtual <-> virtual changes between versions
bool addNullcheck = true;
if (callInfo->nullInstanceCheck)
{
call->gtFlags |= GTF_CALL_NULLCHECK;
GenTree* thisArg = impStackTop(sig->numArgs).val;
if (thisArg->OperIs(GT_RET_EXPR))
{
GenTreeCall* thisAsCall = thisArg->AsRetExpr()->gtInlineCandidate->AsCall();
if (thisAsCall->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC)
{
NamedIntrinsic ni = lookupNamedIntrinsic(thisAsCall->gtCallMethHnd);
if ((ni == NI_System_Collections_Generic_EqualityComparer_get_Default) ||
(ni == NI_System_Collections_Generic_Comparer_get_Default))
{
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice to re-use the pattern in fgIsCurrentCallDceCandidate here -- maybe generalize that to take a method handle?

Copy link
Member Author

Choose a reason for hiding this comment

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

Technically this is a bit different logic, it's a quick check that we can skip a nullcheck, we can add here more intrinsics, potentially:

NI_System_Threading_Thread_get_CurrentThread
NI_System_Array_Clone
NI_System_Object_MemberwiseClone
NI_System_Object_GetType

all of these are not expected to return null afair

Copy link
Member

Choose a reason for hiding this comment

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

Got it.

We should perhaps create a class similar to HelperCallProperties where we can capture this sort of thing in one place; it may be useful to know elsewhere.

addNullcheck = false;
}
}
}

if (addNullcheck)
{
call->gtFlags |= GTF_CALL_NULLCHECK;
}
}
}
#endif
Expand Down