-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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.
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
Late cast expansion: more improvements #97387
Conversation
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch Issue DetailsThis PR:
Almost final step prior getting rid of the importer's cast expansion.
|
/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress |
Azure Pipelines successfully started running 3 pipeline(s). |
/azp run runtime-coreclr pgo, runtime-coreclr libraries-pgo, runtime-coreclr pgostress |
Azure Pipelines successfully started running 3 pipeline(s). |
PTAL @jakobbotsch since you already reviewed this logc, cc @dotnet/jit-contrib this phase is already capable of expanding all kinds of casts and can replace importer, but I need to resolve a couple of CQ nits first before I remove importer. Quite a few changes but the main idea is to put all legality and profitability checks to CI failures aren't related. it also passes the tests if I disable importer |
src/coreclr/jit/eeinterface.cpp
Outdated
// Note: | ||
// We are conservative on arrays of primitive types here. | ||
// | ||
bool Compiler::eeIsClassExact(CORINFO_CLASS_HANDLE clsHnd) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll remove this method because Jan already moved it to VM side in #97424
src/coreclr/jit/helperexpansion.cpp
Outdated
// Likely class handle or NO_CLASS_HANDLE | ||
// | ||
static CORINFO_CLASS_HANDLE PickLikelyClass(Compiler* comp, IL_OFFSET offset, unsigned* likelihood) | ||
enum TypeCheckFailedAction |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Make it an enum class
and remove the prefixes below?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, indeed, addressed
src/coreclr/jit/helperexpansion.cpp
Outdated
enum TypeCheckPassedAction | ||
{ | ||
P_ReturnObj, | ||
P_ReturnNull, | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
src/coreclr/jit/helperexpansion.cpp
Outdated
// comp - Compiler instance | ||
// castHelper - Cast helper call to expand | ||
// commonCls - [out] Common denominator class for the fast and the fallback paths. | ||
// likelihood - [out] Likelihood of successful type check (0..100) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't it be inclusive in both ends?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
addressed
// These are never expanded: | ||
// CORINFO_HELP_ISINSTANCEOF_EXCEPTION | ||
// CORINFO_HELP_CHKCASTCLASS_SPECIAL | ||
// CORINFO_HELP_READYTORUN_ISINSTANCEOF, | ||
// CORINFO_HELP_READYTORUN_CHKCAST, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just for my curiosity, why not expand the R2R version if we have static PGO?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, but importer also always gives up on them currently, but I'll add it to my todo
src/coreclr/jit/helperexpansion.cpp
Outdated
if ((comp->info.compCompHnd->getClassAttribs(result) & | ||
(CORINFO_FLG_INTERFACE | CORINFO_FLG_ABSTRACT | CORINFO_FLG_STATIC)) != 0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is CORINFO_FLG_STATIC
possible on a class? I think it only applies to methods. Static classes in C# are abstract and sealed in IL, I think.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Absolutely not needed, no idea why I added it here. this check was initially added to handle stale static pgo
src/coreclr/jit/helperexpansion.cpp
Outdated
if (typeCheckFailedAction == F_CallHelper_AlwaysThrows) | ||
{ | ||
// fallback call is used only to throw InvalidCastException | ||
fallbackBb = fgNewBBFromTreeAfter(BBJ_THROW, typeCheckBb, gtUnusedValNode(call), debugInfo, nullptr, true); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think gtUnusedValNode
is necessary. Also, shouldn't we mark the call with GTF_CALL_M_DOES_NOT_RETURN
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
src/coreclr/jit/helperexpansion.cpp
Outdated
else if (typeCheckFailedAction == F_ReturnNull) | ||
{ | ||
// if fallback call is not needed, we just assign null to tmp | ||
GenTree* fallbackTree = gtNewTempStore(tmpNum, gtNewNull()); | ||
fallbackBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, typeCheckBb, fallbackTree, debugInfo, lastBb, true); | ||
} | ||
else | ||
{ | ||
GenTree* fallbackTree = gtNewTempStore(tmpNum, call); | ||
fallbackBb = fgNewBBFromTreeAfter(BBJ_ALWAYS, typeCheckBb, fallbackTree, debugInfo, lastBb, true); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems the nice comment above is no longer fully accurate since the fallback BB can also assign null in some cases now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This generally looks great to me, I just had a few nits. You can consider running a SPMI collection on this branch to be able to get proper SPMI diffs (maybe once Jan's PR has been merged)
Diff results for #97387Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,494,760 contexts (1,003,806 MinOpts, 1,490,954 FullOpts). MISSED contexts: base: 4,060 (0.16%), diff: 10,457 (0.42%) Overall (+80 bytes)
FullOpts (+80 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,588,583 contexts (1,052,329 MinOpts, 1,536,254 FullOpts). MISSED contexts: base: 3,628 (0.14%), diff: 10,052 (0.39%) Overall (+230 bytes)
FullOpts (+230 bytes)
Assembly diffs for osx/arm64 ran on windows/x64Diffs are based on 2,256,559 contexts (930,876 MinOpts, 1,325,683 FullOpts). MISSED contexts: base: 3,256 (0.14%), diff: 9,406 (0.42%) Overall (-16 bytes)
FullOpts (-16 bytes)
Assembly diffs for windows/arm64 ran on windows/x64Diffs are based on 2,312,039 contexts (931,543 MinOpts, 1,380,496 FullOpts). MISSED contexts: base: 2,687 (0.12%), diff: 8,855 (0.38%) Overall (-80 bytes)
FullOpts (-80 bytes)
Assembly diffs for windows/x64 ran on windows/x64Diffs are based on 2,486,103 contexts (983,689 MinOpts, 1,502,414 FullOpts). MISSED contexts: base: 3,899 (0.16%), diff: 10,701 (0.43%) Overall (+256 bytes)
FullOpts (+256 bytes)
Details here Assembly diffs for linux/arm ran on windows/x86Diffs are based on 2,231,354 contexts (827,812 MinOpts, 1,403,542 FullOpts). MISSED contexts: base: 74,588 (3.23%), diff: 80,924 (3.50%) Overall (-6 bytes)
FullOpts (-6 bytes)
Assembly diffs for windows/x86 ran on windows/x86Diffs are based on 2,289,781 contexts (841,817 MinOpts, 1,447,964 FullOpts). MISSED contexts: base: 5,093 (0.22%), diff: 11,589 (0.50%) Overall (+144 bytes)
FullOpts (+144 bytes)
Details here |
…st-expansion-2 # Conflicts: # src/coreclr/jit/gentree.cpp # src/coreclr/jit/importer.cpp
Diff results for #97387Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,494,760 contexts (1,003,806 MinOpts, 1,490,954 FullOpts). MISSED contexts: base: 4,060 (0.16%), diff: 10,457 (0.42%) Overall (+80 bytes)
FullOpts (+80 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,588,583 contexts (1,052,329 MinOpts, 1,536,254 FullOpts). MISSED contexts: base: 3,628 (0.14%), diff: 10,052 (0.39%) Overall (+230 bytes)
FullOpts (+230 bytes)
Assembly diffs for osx/arm64 ran on windows/x64Diffs are based on 2,256,559 contexts (930,876 MinOpts, 1,325,683 FullOpts). MISSED contexts: base: 3,256 (0.14%), diff: 9,406 (0.42%) Overall (-16 bytes)
FullOpts (-16 bytes)
Assembly diffs for windows/arm64 ran on windows/x64Diffs are based on 2,312,039 contexts (931,543 MinOpts, 1,380,496 FullOpts). MISSED contexts: base: 2,687 (0.12%), diff: 8,855 (0.38%) Overall (-80 bytes)
FullOpts (-80 bytes)
Assembly diffs for windows/x64 ran on windows/x64Diffs are based on 2,486,103 contexts (983,689 MinOpts, 1,502,414 FullOpts). MISSED contexts: base: 3,899 (0.16%), diff: 10,701 (0.43%) Overall (+256 bytes)
FullOpts (+256 bytes)
Details here Assembly diffs for linux/arm ran on windows/x86Diffs are based on 2,231,354 contexts (827,812 MinOpts, 1,403,542 FullOpts). MISSED contexts: base: 74,588 (3.23%), diff: 80,924 (3.50%) Overall (-6 bytes)
FullOpts (-6 bytes)
Assembly diffs for windows/x86 ran on windows/x86Diffs are based on 2,289,781 contexts (841,817 MinOpts, 1,447,964 FullOpts). MISSED contexts: base: 5,093 (0.22%), diff: 11,589 (0.50%) Overall (+144 bytes)
FullOpts (+144 bytes)
Details here |
Diff results for #97387Assembly diffsAssembly diffs for windows/arm64 ran on linux/x64Diffs are based on 2,308,464 contexts (929,692 MinOpts, 1,378,772 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6,334 (0.27%) Overall (-68 bytes)
FullOpts (-68 bytes)
Details here |
Diff results for #97387Assembly diffsAssembly diffs for linux/arm64 ran on windows/x64Diffs are based on 2,498,787 contexts (1,011,240 MinOpts, 1,487,547 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6,564 (0.26%) Overall (-16 bytes)
FullOpts (-16 bytes)
Assembly diffs for linux/x64 ran on windows/x64Diffs are based on 2,505,538 contexts (977,780 MinOpts, 1,527,758 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6,724 (0.27%) Overall (+159 bytes)
FullOpts (+159 bytes)
Assembly diffs for windows/arm64 ran on windows/x64Diffs are based on 2,308,464 contexts (929,692 MinOpts, 1,378,772 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6,334 (0.27%) Overall (-68 bytes)
FullOpts (-68 bytes)
Assembly diffs for windows/x64 ran on windows/x64Diffs are based on 2,366,596 contexts (928,756 MinOpts, 1,437,840 FullOpts). MISSED contexts: base: 0 (0.00%), diff: 6,605 (0.28%) Overall (+199 bytes)
FullOpts (+199 bytes)
Details here Assembly diffs for linux/arm ran on windows/x86Diffs are based on 2,230,531 contexts (825,130 MinOpts, 1,405,401 FullOpts). MISSED contexts: base: 70,976 (3.08%), diff: 77,526 (3.36%) Overall (-22 bytes)
FullOpts (-22 bytes)
Assembly diffs for windows/x86 ran on windows/x86Diffs are based on 2,292,458 contexts (840,463 MinOpts, 1,451,995 FullOpts). MISSED contexts: base: 7 (0.00%), diff: 6,670 (0.29%) Overall (+127 bytes)
FullOpts (+127 bytes)
Details here |
This PR:
impIsClassExact
toeeIsClassExact
fgLateCastExpansion
to move all logic to a separatePickCandidateForTypeCheck
that performs all legality and profitability checks and gives a strategy for thefgLateCastExpansionForCall
on how to properly expand everything.Almost final step prior getting rid of the importer's cast expansion.