Skip to content

Commit

Permalink
Revert "JIT: Drop redundant static initializations from (Equality)Com…
Browse files Browse the repository at this point in the history 
…parer<T>.Default (dotnet#50446)"

This reverts commit 46e5fe1.
  • Loading branch information
@EgorBo
EgorBo committed Jul 27, 2021
1 parent c81ca16 commit 7120dee
Show file tree
Hide file tree
Showing 9 changed files with 183 additions and 114 deletions.
4 changes: 3 additions & 1 deletion src/coreclr/jit/compiler.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -4053,6 +4053,8 @@ class Compiler
CORINFO_CALL_INFO* callInfo,
IL_OFFSET rawILOffset);

CORINFO_CLASS_HANDLE impGetSpecialIntrinsicExactReturnType(CORINFO_METHOD_HANDLE specialIntrinsicHandle);

bool impMethodInfo_hasRetBuffArg(CORINFO_METHOD_INFO* methInfo, CorInfoCallConvExtension callConv);

GenTree* impFixupCallStructReturn(GenTreeCall* call, CORINFO_CLASS_HANDLE retClsHnd);
Expand Down Expand Up @@ -4100,6 +4102,7 @@ class Compiler
var_types callType,
NamedIntrinsic intrinsicName,
bool tailCall);
NamedIntrinsic lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method);
GenTree* impUnsupportedNamedIntrinsic(unsigned helper,
CORINFO_METHOD_HANDLE method,
CORINFO_SIG_INFO* sig,
Expand Down Expand Up @@ -4200,7 +4203,6 @@ class Compiler
CHECK_SPILL_NONE = -2
};

NamedIntrinsic lookupNamedIntrinsic(CORINFO_METHOD_HANDLE method);
void impBeginTreeList();
void impEndTreeList(BasicBlock* block, Statement* firstStmt, Statement* lastStmt);
void impEndTreeList(BasicBlock* block);
Expand Down
93 changes: 82 additions & 11 deletions src/coreclr/jit/fginline.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1623,7 +1623,8 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
if (argInfo.argHasSideEff)
{
noway_assert(argInfo.argIsUsed == false);
newStmt = nullptr;
newStmt = nullptr;
bool append = true;

if (argNode->gtOper == GT_OBJ || argNode->gtOper == GT_MKREFANY)
{
Expand All @@ -1632,22 +1633,92 @@ Statement* Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
// Just hang the address here in case there are side-effect.
newStmt = gtNewStmt(gtUnusedValNode(argNode->AsOp()->gtOp1), callILOffset);
}

// If we don't have something custom to append,
// just append the arg node as an unused value.
if (newStmt == nullptr)
else
{
newStmt = gtNewStmt(gtUnusedValNode(argNode), callILOffset);
// In some special cases, unused args with side effects can
// trigger further changes.
//
// (1) If the arg is a static field access and the field access
// was produced by a call to EqualityComparer<T>.get_Default, the
// helper call to ensure the field has a value can be suppressed.
// This helper call is marked as a "Special DCE" helper during
// importation, over in fgGetStaticsCCtorHelper.
//
// (2) NYI. If, after tunneling through GT_RET_VALs, we find that
// the actual arg expression has no side effects, we can skip
// appending all together. This will help jit TP a bit.
//
// Chase through any GT_RET_EXPRs to find the actual argument
// expression.
GenTree* actualArgNode = argNode->gtRetExprVal(&bbFlags);

// 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 (actualArgNode->gtOper == GT_COMMA)
{
// Look for (COMMA (CALL(special dce helper...), (FIELD ...)))
GenTree* op1 = actualArgNode->AsOp()->gtOp1;
GenTree* op2 = actualArgNode->AsOp()->gtOp2;
if (op1->IsCall() &&
((op1->AsCall()->gtCallMoreFlags & GTF_CALL_M_HELPER_SPECIAL_DCE) != 0) &&
(op2->gtOper == GT_FIELD) && ((op2->gtFlags & GTF_EXCEPT) == 0))
{
JITDUMP("\nPerforming special dce on unused arg [%06u]:"
" actual arg [%06u] helper call [%06u]\n",
argNode->gtTreeID, actualArgNode->gtTreeID, op1->gtTreeID);
// Drop the whole tree
append = false;
}
}
else if (actualArgNode->gtOper == GT_IND)
{
// Look for (IND (ADD (CONST, CALL(special dce helper...))))
GenTree* addr = actualArgNode->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, actualArgNode->gtTreeID, op1->gtTreeID);
append = false;
}
}
}
}

fgInsertStmtAfter(block, afterStmt, newStmt);
afterStmt = newStmt;
#ifdef DEBUG
if (verbose)
if (!append)
{
gtDispStmt(afterStmt);
assert(newStmt == nullptr);
JITDUMP("Arg tree side effects were discardable, not appending anything for arg\n");
}
else
{
// If we don't have something custom to append,
// just append the arg node as an unused value.
if (newStmt == nullptr)
{
newStmt = gtNewStmt(gtUnusedValNode(argNode), callILOffset);
}

fgInsertStmtAfter(block, afterStmt, newStmt);
afterStmt = newStmt;
#ifdef DEBUG
if (verbose)
{
gtDispStmt(afterStmt);
}
#endif // DEBUG
}
}
else if (argNode->IsBoxedValue())
{
Expand Down
15 changes: 15 additions & 0 deletions src/coreclr/jit/flowgraph.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -910,6 +910,21 @@ GenTreeCall* Compiler::fgGetStaticsCCtorHelper(CORINFO_CLASS_HANDLE cls, CorInfo

GenTreeCall* result = gtNewHelperCallNode(helper, type, argList);
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_JIT_INTRINSIC) != 0)
{
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;
}
}

return result;
}

Expand Down
65 changes: 8 additions & 57 deletions src/coreclr/jit/gentree.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -882,17 +882,6 @@ bool GenTreeCall::IsPure(Compiler* compiler) const
// true if this call has any side-effects; false otherwise.
bool GenTreeCall::HasSideEffects(Compiler* compiler, bool ignoreExceptions, bool ignoreCctors) const
{
// Some named intrinsics are known to have ignorable side effects.
if (gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC)
{
NamedIntrinsic ni = compiler->lookupNamedIntrinsic(gtCallMethHnd);
if ((ni == NI_System_Collections_Generic_Comparer_get_Default) ||
(ni == NI_System_Collections_Generic_EqualityComparer_get_Default))
{
return false;
}
}

// Generally all GT_CALL nodes are considered to have side-effects, but we may have extra information about helper
// calls that can prove them side-effect-free.
if (gtCallType != CT_HELPER)
Expand Down Expand Up @@ -15981,10 +15970,9 @@ void Compiler::gtExtractSideEffList(GenTree* expr,
}

// Generally all GT_CALL nodes are considered to have side-effects.
// So if we get here it must be a helper call or a special intrinsic that we decided it does
// So if we get here it must be a helper call that we decided it does
// not have side effects that we needed to keep.
assert(!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER) ||
(node->AsCall()->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC));
assert(!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER));
}

if ((m_flags & GTF_IS_IN_CSE) != 0)
Expand Down Expand Up @@ -17805,50 +17793,13 @@ CORINFO_CLASS_HANDLE Compiler::gtGetClassHandle(GenTree* tree, bool* pIsExact, b
break;
}

// Try to get the actual type of [Equality]Comparer<>.Default
if ((ni == NI_System_Collections_Generic_Comparer_get_Default) ||
(ni == NI_System_Collections_Generic_EqualityComparer_get_Default))
CORINFO_CLASS_HANDLE specialObjClass = impGetSpecialIntrinsicExactReturnType(call->gtCallMethHnd);
if (specialObjClass != nullptr)
{
CORINFO_SIG_INFO sig;
info.compCompHnd->getMethodSig(call->gtCallMethHnd, &sig);
assert(sig.sigInst.classInstCount == 1);
CORINFO_CLASS_HANDLE typeHnd = sig.sigInst.classInst[0];
assert(typeHnd != nullptr);

// Lookup can incorrect when we have __Canon as it won't appear to implement any interface types.
// And if we do not have a final type, devirt & inlining is unlikely to result in much
// simplification. We can use CORINFO_FLG_FINAL to screen out both of these cases.
const DWORD typeAttribs = info.compCompHnd->getClassAttribs(typeHnd);
const bool isFinalType = ((typeAttribs & CORINFO_FLG_FINAL) != 0);

if (isFinalType)
{
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
{
objClass = info.compCompHnd->getDefaultEqualityComparerClass(typeHnd);
}
else
{
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
objClass = info.compCompHnd->getDefaultComparerClass(typeHnd);
}
}

if (objClass == nullptr)
{
// Don't re-visit this intrinsic in this case.
call->gtCallMoreFlags &= ~GTF_CALL_M_SPECIAL_INTRINSIC;
JITDUMP("Special intrinsic for type %s: type not final, so deferring opt\n",
eeGetClassName(typeHnd))
}
else
{
JITDUMP("Special intrinsic for type %s: return type is %s\n", eeGetClassName(typeHnd),
eeGetClassName(objClass))
*pIsExact = true;
*pIsNonNull = true;
break;
}
objClass = specialObjClass;
*pIsExact = true;
*pIsNonNull = true;
break;
}
}
if (call->IsInlineCandidate())
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/gentree.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3797,6 +3797,8 @@ enum GenTreeCallFlags : unsigned int
// to restore real function address and load hidden argument
// as the first argument for calli. It is CoreRT replacement for instantiating
// stubs, because executable code cannot be generated at runtime.
GTF_CALL_M_HELPER_SPECIAL_DCE = 0x00020000, // this helper call can be removed if it is part of a comma and
// the comma result is unused.
GTF_CALL_M_DEVIRTUALIZED = 0x00040000, // this call was devirtualized
GTF_CALL_M_UNBOXED = 0x00080000, // this call was optimized to use the unboxed entry point
GTF_CALL_M_GUARDED_DEVIRT = 0x00100000, // this call is a candidate for guarded devirtualization
Expand Down
86 changes: 73 additions & 13 deletions src/coreclr/jit/importer.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19332,19 +19332,6 @@ void Compiler::impCheckCanInline(GenTreeCall* call,
goto _exit;
}

// It's better for JIT to keep these methods not inlined for CQ.
NamedIntrinsic ni;
if (pParam->call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC)
{
ni = pParam->pThis->lookupNamedIntrinsic(pParam->call->gtCallMethHnd);
if ((ni == NI_System_Collections_Generic_Comparer_get_Default) ||
(ni == NI_System_Collections_Generic_EqualityComparer_get_Default))
{
pParam->result->NoteFatal(InlineObservation::CALLEE_SPECIAL_INTRINSIC);
goto _exit;
}
}

// Speculatively check if initClass() can be done.
// If it can be done, we will try to inline the method.
initClassResult =
Expand Down Expand Up @@ -21603,6 +21590,79 @@ void Compiler::impDevirtualizeCall(GenTreeCall* call,
#endif // FEATURE_READYTORUN_COMPILER
}

//------------------------------------------------------------------------
// impGetSpecialIntrinsicExactReturnType: Look for special cases where a call
// to an intrinsic returns an exact type
//
// Arguments:
// methodHnd -- handle for the special intrinsic method
//
// Returns:
// Exact class handle returned by the intrinsic call, if known.
// Nullptr if not known, or not likely to lead to beneficial optimization.

CORINFO_CLASS_HANDLE Compiler::impGetSpecialIntrinsicExactReturnType(CORINFO_METHOD_HANDLE methodHnd)
{
JITDUMP("Special intrinsic: looking for exact type returned by %s\n", eeGetMethodFullName(methodHnd));

CORINFO_CLASS_HANDLE result = nullptr;

// See what intrinisc we have...
const NamedIntrinsic ni = lookupNamedIntrinsic(methodHnd);
switch (ni)
{
case NI_System_Collections_Generic_Comparer_get_Default:
case NI_System_Collections_Generic_EqualityComparer_get_Default:
{
// Expect one class generic parameter; figure out which it is.
CORINFO_SIG_INFO sig;
info.compCompHnd->getMethodSig(methodHnd, &sig);
assert(sig.sigInst.classInstCount == 1);
CORINFO_CLASS_HANDLE typeHnd = sig.sigInst.classInst[0];
assert(typeHnd != nullptr);

// Lookup can incorrect when we have __Canon as it won't appear
// to implement any interface types.
//
// And if we do not have a final type, devirt & inlining is
// unlikely to result in much simplification.
//
// We can use CORINFO_FLG_FINAL to screen out both of these cases.
const DWORD typeAttribs = info.compCompHnd->getClassAttribs(typeHnd);
const bool isFinalType = ((typeAttribs & CORINFO_FLG_FINAL) != 0);

if (isFinalType)
{
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
{
result = info.compCompHnd->getDefaultEqualityComparerClass(typeHnd);
}
else
{
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
result = info.compCompHnd->getDefaultComparerClass(typeHnd);
}
JITDUMP("Special intrinsic for type %s: return type is %s\n", eeGetClassName(typeHnd),
result != nullptr ? eeGetClassName(result) : "unknown");
}
else
{
JITDUMP("Special intrinsic for type %s: type not final, so deferring opt\n", eeGetClassName(typeHnd));
}

break;
}

default:
{
JITDUMP("This special intrinsic not handled, sorry...\n");
break;
}
}

return result;
}

//------------------------------------------------------------------------
// impAllocateToken: create CORINFO_RESOLVED_TOKEN into jit-allocated memory and init it.
//
Expand Down
1 change: 0 additions & 1 deletion src/coreclr/jit/inline.def
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,7 +57,6 @@ INLINE_OBSERVATION(STFLD_NEEDS_HELPER, bool, "stfld needs helper",
INLINE_OBSERVATION(TOO_MANY_ARGUMENTS, bool, "too many arguments", FATAL, CALLEE)
INLINE_OBSERVATION(TOO_MANY_LOCALS, bool, "too many locals", FATAL, CALLEE)
INLINE_OBSERVATION(EXPLICIT_TAIL_PREFIX, bool, "explicit tail prefix in callee", FATAL, CALLEE)
INLINE_OBSERVATION(SPECIAL_INTRINSIC, bool, "skipped for special intrinsic", FATAL, CALLEE)

// ------ Callee Performance -------

Expand Down
28 changes: 0 additions & 28 deletions src/coreclr/jit/valuenum.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9676,34 +9676,6 @@ void Compiler::fgValueNumberCall(GenTreeCall* call)
}
else
{
if (call->gtCallMoreFlags & GTF_CALL_M_SPECIAL_INTRINSIC)
{
NamedIntrinsic ni = lookupNamedIntrinsic(call->gtCallMethHnd);
if ((ni == NI_System_Collections_Generic_Comparer_get_Default) ||
(ni == NI_System_Collections_Generic_EqualityComparer_get_Default))
{
bool isExact = false;
bool isNotNull = false;
CORINFO_CLASS_HANDLE cls = gtGetClassHandle(call, &isExact, &isNotNull);
if ((cls != nullptr) && isExact && isNotNull)
{
ValueNum clsVN = vnStore->VNForHandle(ssize_t(cls), GTF_ICON_CLASS_HDL);
ValueNum funcVN;
if (ni == NI_System_Collections_Generic_EqualityComparer_get_Default)
{
funcVN = vnStore->VNForFunc(call->TypeGet(), VNF_GetDefaultEqualityComparer, clsVN);
}
else
{
assert(ni == NI_System_Collections_Generic_Comparer_get_Default);
funcVN = vnStore->VNForFunc(call->TypeGet(), VNF_GetDefaultComparer, clsVN);
}
call->gtVNPair.SetBoth(funcVN);
return;
}
}
}

if (call->TypeGet() == TYP_VOID)
{
call->gtVNPair.SetBoth(ValueNumStore::VNForVoid());
Expand Down
Loading

0 comments on commit 7120dee

Please sign in to comment.