Skip to content

JIT: Boost inlining for more derived returns #113256

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

Closed
wants to merge 6 commits into from
+81 −1
Closed

JIT: Boost inlining for more derived returns #113256

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
2864228
Boost inlining for more derived returns
hez2010 Mar 7, 2025
ab95f51
Handle BOX
hez2010 Mar 7, 2025
a4d4914
Merge branch 'main' into exact-inlining-boost
hez2010 Mar 7, 2025
0411154
Remove unused methodFlags
hez2010 Mar 7, 2025
f16123f
Skip known exact or void return
hez2010 Mar 7, 2025
4a30d9e
Remove wrapper method check and adjust multiplier
hez2010 Mar 10, 2025
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
63 changes: 62 additions & 1 deletion src/coreclr/jit/fgbasic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -735,7 +735,8 @@ class FgStack
SLOT_UNKNOWN = 0,
SLOT_CONSTANT = 1,
SLOT_ARRAYLEN = 2,
SLOT_ARGUMENT = 3
SLOT_EXACTCLS = 3,
SLOT_ARGUMENT = 4,
};

void Clear()
Expand All @@ -754,6 +755,10 @@ class FgStack
{
Push(SLOT_ARRAYLEN);
}
void PushExactClass()
{
Push(SLOT_EXACTCLS);
}
void PushArgument(unsigned arg)
{
Push((FgSlot)(SLOT_ARGUMENT + arg));
Expand Down Expand Up @@ -790,6 +795,10 @@ class FgStack
{
return value == SLOT_ARRAYLEN;
}
static bool IsExactClass(FgSlot value)
{
return value == SLOT_EXACTCLS;
}
static bool IsArgument(FgSlot value)
{
return value >= SLOT_ARGUMENT;
Expand Down Expand Up @@ -891,6 +900,7 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
int prefixFlags = 0;
bool preciseScan = makeInlineObservations && compInlineResult->GetPolicy()->RequiresPreciseScan();
const bool resolveTokens = preciseScan;
bool exactSigRet = true;

// Track offsets where IL instructions begin in DEBUG builds. Used to
// validate debug info generated by the JIT.
Expand Down Expand Up @@ -939,6 +949,16 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
compInlineResult->Note(InlineObservation::CALLEE_BEGIN_OPCODE_SCAN);
}

if (preciseScan)
{
CORINFO_SIG_INFO sig;
eeGetMethodSig(info.compMethodHnd, &sig);
if (sig.retType == CORINFO_TYPE_CLASS)
{
exactSigRet = info.compCompHnd->isExactType(sig.retTypeClass);
}
Copy link
Member

@EgorBo EgorBo Mar 7, 2025

Choose a reason for hiding this comment

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

We avoid calling VM apis during inlining - this potentially can be slow/trigger type load events for every inline attempt (and we do many of them)

Copy link
Member

@EgorBo EgorBo Mar 7, 2025

Choose a reason for hiding this comment

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

I meant not this one, but eeGetMethodSig(methodHnd, &methodSig); below

Copy link
Contributor Author

@hez2010 hez2010 Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this potentially can be slow/trigger type load events for every inline attempt (and we do many of them)

We are already resolving method tokens for calls while scanning the inlinee, which loads all the type dependencies, so I don't see this to be a problem.

Copy link
Member

@EgorBo EgorBo Mar 7, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

this potentially can be slow/trigger type load events for every inline attempt (and we do many of them)

We are already resolving method tokens for calls while scanning the inlinee, which loads all the type dependencies, so I don't see this to be a problem.

Yes and the initial impl of that caused a massive TP regression as far as I remember, so it's not an excuse to continue calling other VM stuff, if we want to land this, we need to measure/make sure it's cheap.

The main bottleneck in application startup are VM calls triggered by JIT's importer

}

CORINFO_RESOLVED_TOKEN resolvedToken;

OPCODE opcode = CEE_NOP;
Expand Down Expand Up @@ -1053,6 +1073,13 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
}
}
}

if (preciseScan && FgStack::IsExactClass(pushedStack.Top()))
{
// Box a struct results in an exact class
handled = true;
}

break;
}

Expand Down Expand Up @@ -1112,12 +1139,14 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
CORINFO_METHOD_HANDLE methodHnd = nullptr;
bool isIntrinsic = false;
NamedIntrinsic ni = NI_Illegal;
CORINFO_SIG_INFO methodSig = {};

if (resolveTokens)
{
impResolveToken(codeAddr, &resolvedToken, CORINFO_TOKENKIND_Method);
methodHnd = resolvedToken.hMethod;
isIntrinsic = eeIsIntrinsic(methodHnd);
eeGetMethodSig(methodHnd, &methodSig);
}

if (isIntrinsic)
Expand Down Expand Up @@ -1577,6 +1606,13 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
}
}
}
else if (resolveTokens && (methodSig.retType != CORINFO_TYPE_CLASS ||
info.compCompHnd->isExactType(methodSig.retTypeClass)))
{
// The method returns an object of an exact class.
pushedStack.PushExactClass();
handled = true;
}

if ((codeAddr < codeEndp - sz) && (OPCODE)getU1LittleEndian(codeAddr + sz) == CEE_RET)
{
Expand All @@ -1594,6 +1630,27 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
}
break;

case CEE_NEWARR:
case CEE_NEWOBJ:
case CEE_INITOBJ:
{
if (preciseScan)
{
// A newobj/newarr/initobj always returns an object of an exact class.
pushedStack.PushExactClass();
handled = true;
}

if (makeInlineObservations && (codeAddr < codeEndp - sz) &&
(OPCODE)getU1LittleEndian(codeAddr + sz) == CEE_RET)
{
// If the method has a newobj/newarr/initobj followed by a ret, assume that
// it is a wrapper method.
compInlineResult->Note(InlineObservation::CALLEE_LOOKS_LIKE_WRAPPER);
}
break;
}

case CEE_LDIND_I1:
case CEE_LDIND_U1:
case CEE_LDIND_I2:
Expand Down Expand Up @@ -2408,6 +2465,10 @@ void Compiler::fgFindJumpTargets(const BYTE* codeAddr, IL_OFFSET codeSize, Fixed
break;

case CEE_RET:
if (makeInlineObservations && !exactSigRet && FgStack::IsExactClass(pushedStack.Top()))
{
compInlineResult->Note(InlineObservation::CALLEE_MORE_DERIVED_RETURNS);
}
retBlocks++;
break;

Expand Down
1 change: 1 addition & 0 deletions src/coreclr/jit/inline.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,7 @@ INLINE_OBSERVATION(NUMBER_OF_BASIC_BLOCKS, int, "number of basic blocks",
INLINE_OBSERVATION(NUMBER_OF_LOCALS, int, "number of locals", INFORMATION, CALLEE)
INLINE_OBSERVATION(RANDOM_ACCEPT, bool, "random accept", INFORMATION, CALLEE)
INLINE_OBSERVATION(UNBOX_ARG, int, "callee unboxes arg", INFORMATION, CALLEE)
INLINE_OBSERVATION(MORE_DERIVED_RETURNS, bool, "returns more derived type", INFORMATION, CALLEE)
INLINE_OBSERVATION(UNSUPPORTED_OPCODE, bool, "unsupported opcode", INFORMATION, CALLEE)

// ------ Caller Correctness -------
Expand Down
21 changes: 21 additions & 0 deletions src/coreclr/jit/inlinepolicy.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,6 +328,10 @@ void DefaultPolicy::NoteBool(InlineObservation obs, bool value)
m_ArgFeedsIsKnownConst = true;
break;

case InlineObservation::CALLEE_MORE_DERIVED_RETURNS:
m_CalleeReturnsDerivedType = true;
break;

case InlineObservation::CALLEE_UNSUPPORTED_OPCODE:
propagate = true;
break;
Expand Down Expand Up @@ -714,6 +718,14 @@ double DefaultPolicy::DetermineMultiplier()
JITDUMP("\nmultiplier in instance constructors increased to %g.", multiplier);
}

// Bump up the multiplier for methods that return a more derived type

if (m_CalleeReturnsDerivedType)
{
multiplier += 4;
JITDUMP("\nmultiplier in methods that return a more derived type increased to %g.", multiplier);
}

// Bump up the multiplier for methods in promotable struct

if (m_IsFromPromotableValueClass)
Expand Down Expand Up @@ -1042,6 +1054,7 @@ void DefaultPolicy::OnDumpXml(FILE* file, unsigned indent) const
XATTR_B(m_IsNoReturn)
XATTR_B(m_IsNoReturnKnown)
XATTR_B(m_InsideThrowBlock)
XATTR_B(m_CalleeReturnsDerivedType)
}
#endif

Expand Down Expand Up @@ -1526,6 +1539,12 @@ double ExtendedDefaultPolicy::DetermineMultiplier()
JITDUMP("\nmultiplier in instance constructors increased to %g.", multiplier);
}

if (m_CalleeReturnsDerivedType)
{
multiplier += 4.0;
JITDUMP("\nmultiplier in methods that return a more derived type increased to %g.", multiplier);
}

if (m_IsFromValueClass)
{
multiplier += 3.0;
Expand Down Expand Up @@ -2708,6 +2727,7 @@ void DiscretionaryPolicy::DumpSchema(FILE* file) const
fprintf(file, ",CalleeDoesNotReturn");
fprintf(file, ",CalleeHasGCStruct");
fprintf(file, ",CallsiteDepth");
fprintf(file, ",CalleeReturnsDerivedType");
}

//------------------------------------------------------------------------
Expand Down Expand Up @@ -2792,6 +2812,7 @@ void DiscretionaryPolicy::DumpData(FILE* file) const
fprintf(file, ",%u", m_IsNoReturn ? 1 : 0);
fprintf(file, ",%u", m_CalleeHasGCStruct ? 1 : 0);
fprintf(file, ",%u", m_CallsiteDepth);
fprintf(file, ",%u", m_CalleeReturnsDerivedType ? 1 : 0);
}

#endif // defined(DEBUG)
Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/jit/inlinepolicy.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ class DefaultPolicy : public LegalPolicy
, m_ConstArgFeedsIsKnownConst(false)
, m_ArgFeedsIsKnownConst(false)
, m_InsideThrowBlock(false)
, m_CalleeReturnsDerivedType(false)
{
// empty
}
Expand Down Expand Up @@ -189,6 +190,7 @@ class DefaultPolicy : public LegalPolicy
bool m_ConstArgFeedsIsKnownConst : 1;
bool m_ArgFeedsIsKnownConst : 1;
bool m_InsideThrowBlock : 1;
bool m_CalleeReturnsDerivedType : 1;
};

// ExtendedDefaultPolicy is a slightly more aggressive variant of
Expand Down
Loading