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

JIT: Produce less convoluted IR for boolean isinst checks #103391

Merged
merged 6 commits into from
Jun 18, 2024
+100 −16
Merged

JIT: Produce less convoluted IR for boolean isinst checks #103391

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
14ac08d
JIT: Produce less convoluted IR for boolean `isinst` checks
jakobbotsch Jun 13, 2024
1ca8a25
Use two QMARKs like other case
jakobbotsch Jun 13, 2024
b313ec5
Run jit-format, add function headers
jakobbotsch Jun 13, 2024
0438a0b
Oops
jakobbotsch Jun 13, 2024
c69f319
Feedback
jakobbotsch Jun 17, 2024
cb72437
Dedup code a bit
jakobbotsch Jun 18, 2024
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
4 changes: 3 additions & 1 deletion src/coreclr/jit/compiler.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4774,8 +4774,10 @@ class Compiler
bool impIsCastHelperEligibleForClassProbe(GenTree* tree);
bool impIsCastHelperMayHaveProfileData(CorInfoHelpFunc helper);

bool impMatchIsInstBooleanConversion(const BYTE* codeAddr, const BYTE* codeEndp, int* consumed);

GenTree* impCastClassOrIsInstToTree(
GenTree* op1, GenTree* op2, CORINFO_RESOLVED_TOKEN* pResolvedToken, bool isCastClass, IL_OFFSET ilOffset);
GenTree* op1, GenTree* op2, CORINFO_RESOLVED_TOKEN* pResolvedToken, bool isCastClass, bool* booleanCheck, IL_OFFSET ilOffset);

GenTree* impOptimizeCastClassOrIsInst(GenTree* op1, CORINFO_RESOLVED_TOKEN* pResolvedToken, bool isCastClass);

Expand Down
114 changes: 99 additions & 15 deletions src/coreclr/jit/importer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5464,6 +5464,55 @@ GenTree* Compiler::impOptimizeCastClassOrIsInst(GenTree* op1, CORINFO_RESOLVED_T
return nullptr;
}

//------------------------------------------------------------------------
// impMatchIsInstBooleanConversion: Match IL to determine whether an isinst IL
// instruction is used for a simple boolean check.
//
// Arguments:
// codeAddr - IL after the isinst
// codeEndp - End of IL code stream
// consumed - [out] If this function returns true, set to the number of IL
// bytes to consume to create the boolean check
//
// Return Value:
// True if the isinst is used as a boolean check; otherwise false.
//
// Remarks:
// The isinst instruction is specced to return the original object refernce
// when the type check succeeds. However, in many cases it is used strictly
// as a boolean type check (if (x is Foo) for example). In those cases it is
// beneficial for the JIT if we avoid creating QMARKs returning the object
// itself which may disable some important optimization in some cases.
//
bool Compiler::impMatchIsInstBooleanConversion(const BYTE* codeAddr, const BYTE* codeEndp, int* consumed)
{
OPCODE nextOpcode = impGetNonPrefixOpcode(codeAddr, codeEndp);
switch (nextOpcode)
{
case CEE_BRFALSE:
case CEE_BRFALSE_S:
// BRFALSE/BRTRUE importation are expected to transparently handle
// that the created tree is a TYP_INT instead of TYP_REF, so we do
// not consume them here.
*consumed = 0;
return true;
case CEE_BRTRUE:
case CEE_BRTRUE_S:
jakobbotsch marked this conversation as resolved.
Show resolved Hide resolved
*consumed = 0;
return true;
case CEE_LDNULL:
nextOpcode = impGetNonPrefixOpcode(codeAddr + 1, codeEndp);
if (nextOpcode == CEE_CGT_UN)
EgorBo marked this conversation as resolved.
Show resolved Hide resolved
{
*consumed = 3;
return true;
}
return false;
default:
return false;
}
}

//------------------------------------------------------------------------
// impCastClassOrIsInstToTree: build and import castclass/isinst
//
Expand All @@ -5472,15 +5521,22 @@ GenTree* Compiler::impOptimizeCastClassOrIsInst(GenTree* op1, CORINFO_RESOLVED_T
// op2 - type handle for type to cast to
// pResolvedToken - resolved token from the cast operation
// isCastClass - true if this is castclass, false means isinst
// booleanCheck - [in, out] If true, allow creating a boolean-returning check
// instead of returning the object reference. Set to false if this function
// was not able to create a boolean check.
//
// Return Value:
// Tree representing the cast
//
// Notes:
// May expand into a series of runtime checks or a helper call.
//
GenTree* Compiler::impCastClassOrIsInstToTree(
GenTree* op1, GenTree* op2, CORINFO_RESOLVED_TOKEN* pResolvedToken, bool isCastClass, IL_OFFSET ilOffset)
GenTree* Compiler::impCastClassOrIsInstToTree(GenTree* op1,
GenTree* op2,
CORINFO_RESOLVED_TOKEN* pResolvedToken,
bool isCastClass,
bool* booleanCheck,
IL_OFFSET ilOffset)
{
assert(op1->TypeGet() == TYP_REF);

Expand Down Expand Up @@ -5553,6 +5609,8 @@ GenTree* Compiler::impCastClassOrIsInstToTree(
call->gtCallMoreFlags |= GTF_CALL_M_CAST_CAN_BE_EXPANDED;
call->gtCastHelperILOffset = ilOffset;
}

*booleanCheck = false;
return call;
}

Expand All @@ -5578,19 +5636,37 @@ GenTree* Compiler::impCastClassOrIsInstToTree(
GenTree* op1Clone;
op1 = impCloneExpr(op1, &op1Clone, CHECK_SPILL_ALL, nullptr DEBUGARG("ISINST eval op1"));

GenTreeOp* condMT = gtNewOperNode(GT_NE, TYP_INT, gtNewMethodTableLookup(op1Clone), op2);
GenTreeOp* condNull = gtNewOperNode(GT_EQ, TYP_INT, gtClone(op1), gtNewNull());
GenTreeQmark* qmarkMT = gtNewQmarkNode(TYP_REF, condMT, gtNewColonNode(TYP_REF, gtNewNull(), gtClone(op1)));
GenTreeQmark* qmarkNull = gtNewQmarkNode(TYP_REF, condNull, gtNewColonNode(TYP_REF, gtNewNull(), qmarkMT));
if (*booleanCheck)
{
GenTreeOp* condMT = gtNewOperNode(GT_NE, TYP_INT, gtNewMethodTableLookup(op1Clone), op2);
GenTreeOp* condNull = gtNewOperNode(GT_EQ, TYP_INT, gtClone(op1), gtNewNull());
Copy link
Member

Choose a reason for hiding this comment

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

nit: looks like these two branches have a lot of common code, e.g. condMT and condNull are the same.

Copy link
Member Author

Choose a reason for hiding this comment

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

Deduplicated a bit of it

GenTreeQmark* qmarkMT =
gtNewQmarkNode(TYP_REF, condMT,
gtNewColonNode(TYP_INT, gtNewZeroConNode(TYP_INT), gtNewOneConNode(TYP_INT)));
GenTreeQmark* qmarkNull =
gtNewQmarkNode(TYP_INT, condNull, gtNewColonNode(TYP_INT, gtNewZeroConNode(TYP_INT), qmarkMT));

// Make QMark node a top level node by spilling it.
const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNull"));
impStoreToTemp(result, qmarkNull, CHECK_SPILL_NONE);
// Make QMark node a top level node by spilling it.
const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNull"));
impStoreToTemp(result, qmarkNull, CHECK_SPILL_NONE);
return gtNewLclvNode(result, TYP_INT);
}
else
{
GenTreeOp* condMT = gtNewOperNode(GT_NE, TYP_INT, gtNewMethodTableLookup(op1Clone), op2);
GenTreeOp* condNull = gtNewOperNode(GT_EQ, TYP_INT, gtClone(op1), gtNewNull());
GenTreeQmark* qmarkMT = gtNewQmarkNode(TYP_REF, condMT, gtNewColonNode(TYP_REF, gtNewNull(), gtClone(op1)));
GenTreeQmark* qmarkNull = gtNewQmarkNode(TYP_REF, condNull, gtNewColonNode(TYP_REF, gtNewNull(), qmarkMT));

// Make QMark node a top level node by spilling it.
const unsigned result = lvaGrabTemp(true DEBUGARG("spilling qmarkNull"));
impStoreToTemp(result, qmarkNull, CHECK_SPILL_NONE);

// See also gtGetHelperCallClassHandle where we make the same
// determination for the helper call variants.
lvaSetClass(result, pResolvedToken->hClass);
return gtNewLclvNode(result, TYP_REF);
// See also gtGetHelperCallClassHandle where we make the same
// determination for the helper call variants.
lvaSetClass(result, pResolvedToken->hClass);
return gtNewLclvNode(result, TYP_REF);
}
}

#ifndef DEBUG
Expand Down Expand Up @@ -9626,7 +9702,14 @@ void Compiler::impImportBlockCode(BasicBlock* block)
if (!usingReadyToRunHelper)
#endif
{
op1 = impCastClassOrIsInstToTree(op1, op2, &resolvedToken, false, opcodeOffs);
int consumed = 0;
bool booleanCheck = impMatchIsInstBooleanConversion(codeAddr + sz, codeEndp, &consumed);
op1 = impCastClassOrIsInstToTree(op1, op2, &resolvedToken, false, &booleanCheck, opcodeOffs);

if (booleanCheck)
{
sz += consumed;
}
}
if (compDonotInline())
{
Expand Down Expand Up @@ -10148,7 +10231,8 @@ void Compiler::impImportBlockCode(BasicBlock* block)
if (!usingReadyToRunHelper)
#endif
{
op1 = impCastClassOrIsInstToTree(op1, op2, &resolvedToken, true, opcodeOffs);
bool booleanCheck = false;
op1 = impCastClassOrIsInstToTree(op1, op2, &resolvedToken, true, &booleanCheck, opcodeOffs);
}
if (compDonotInline())
{
Expand Down
Loading