-
Notifications
You must be signed in to change notification settings - Fork 4.5k
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
RyuJIT: Optimize obj.GetType() != typeof(X)
for sealed classes
#32790
Conversation
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.
Thanks for looking at this.
src/coreclr/src/jit/importer.cpp
Outdated
@@ -20742,7 +20756,7 @@ bool Compiler::impCanSkipCovariantStoreCheck(GenTree* value, GenTree* array) | |||
} | |||
|
|||
// Check for T[] with T exact. | |||
if (!impIsClassExact(arrayElementHandle)) | |||
if (!impIsClassExact(arrayElementHandle, false)) | |||
{ |
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.
if I enable impIsClassExact
for arrays by default this call will cause ~400bytes regression. Example: https://www.diffchecker.com/nKWlHaG7
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
This comment was marked as duplicate.
This comment was marked as duplicate.
Sorry, something went wrong.
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.
That's a size regression but quite likely a perf improvement -- the code no longer needs the covariant store check, it just needs a write barrier.
The store check helper also does bounds checking, which now must be done in the jitted code, and is what causes the size growth.
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.
If all the "bad" diffs are like that, I'm ok with a small size regression.
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.
Github acting weird.
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, ok, I thought it introduced a new bound check. Will post the new diff later then.
a32fe1e
to
14f66a4
Compare
@AndyAyersMS
Asm diffs for top3 regressions: the first one looks a bit suspicious (see code), others are the optimized covariance check case. JaggedArray + Vector as T -- doesn't sound like a real world application, so I guess it's pmi. |
In What does the diff in |
Diff: https://www.diffchecker.com/cMI34jXb runtime/src/libraries/System.Private.CoreLib/src/System/Text/UTF32Encoding.cs Lines 1141 to 1145 in 7855aa3
Eliminated that check. |
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 looks good. Thanks!
With this PR, the following snippet: static Span<string> ToSpan(string[] a)
{
return a;
} Asm diff: https://www.diffchecker.com/KtZmiPvY (new codegen is on the right) |
cc @dotnet/jit-contrib |
obj.GetType() != typeof(X)
for sealed classesobj.GetType() != typeof(X)
for sealed classes
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.
LGTM
Motivation: #29093 (comment)
impIsClassExact
now returnstrue
for arrays of sealed classes, e.g.String[]
(but returnsfalse
for arrays of other things including primitives and enums).Also, optimizes
obj.GetType() == typeof(X)
to a const + nullcheck (can be eliminated later by Jit) if we can prove both types are sealed, e.g.:Old codegen for
Check<string>
:New codegen for
Check<string>
:Jit-diff (-f -pmi):
(Correct me if I am wrong but the diff should be way bigger once jit learns how to inline generics into shared methods -- because
Span<T>
uses that check a lot)Will add tests later.
cc @AndyAyersMS