-
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
[RyuJIT] Make casthelpers cold for sealed classes #49295
Conversation
I am wondering - could these cold blocks be safely marked as |
heh, I'm testing it now :-) |
I wonder if we should introduce a new throw helper for this? Among other things we might be able to common the calls (tail merge) which might mitigate some of the code size increase you are seeing. |
From my understanding, all of our corinfo throw helpers are paramterless - and this one requires arguments:
so we can't merge same call but with different arguments. |
New diff is
for other stuff the diff is small because the change introduces a very little size diff (0-few bytes). e.g. for this case: string Test(object o) => (string)o; old codegen: ; Method Pr:Test_sealed(System.Object):System.String:this
G_M52918_IG01:
sub rsp, 40
G_M52918_IG02:
mov rax, rdx
test rax, rax
je SHORT G_M52918_IG05
G_M52918_IG03:
mov rcx, 0xD1FFAB1E
cmp qword ptr [rax], rcx
je SHORT G_M52918_IG05
G_M52918_IG04:
call CORINFO_HELP_CHKCASTCLASS_SPECIAL
G_M52918_IG05:
nop
G_M52918_IG06:
add rsp, 40
ret
; Total bytes of code: 38 new codegen: ; Method Pr:Test_sealed(System.Object):System.String:this
G_M52918_IG01:
sub rsp, 40
G_M52918_IG02:
mov rax, rdx
test rax, rax
je SHORT G_M52918_IG04
G_M52918_IG03:
mov rcx, 0xD1FFAB1E
cmp qword ptr [rax], rcx
jne SHORT G_M52918_IG05
G_M52918_IG04:
add rsp, 40
ret
G_M52918_IG05:
call CORINFO_HELP_CHKCASTCLASS_SPECIAL
int3
; Total bytes of code: 38
|
So the size regressions in say |
Here is the diff: https://www.diffchecker.com/7cVv3EsU I can take a look on why tails weren't merged or can file an issue for it and do it later if it already meets the bar to be merged as is now. |
Presumably it is this method: https://github.com/dotnet/roslyn/blob/6da1274c9d24c2f90a48290394a951b23617f2a3/src/Compilers/CSharp/Portable/BoundTree/BoundTreeVisitors.cs#L30-L42 where all the |
@AndyAyersMS no, it's a similar method but from VisualBasic 🙂 https://github.com/dotnet/roslyn/blob/6da1274c9d24c2f90a48290394a951b23617f2a3/src/Compilers/VisualBasic/Portable/BoundTree/BoundTreeVisitor.vb |
Regarding the |
Seems like the |
So the actual method from the diff is this one: https://github.com/dotnet/roslyn/blob/main/src/Compilers/VisualBasic/Portable/Generated/BoundNodes.xml.Generated.vb#L9290 it has a number of CType(variable, SomeRefClass) which are guarded by a custom enum (JIT doesn't know it relates to the underlying type) - so the cast helpers are valid. |
Got it. Thanks. |
* upstream/main: (83 commits) Fix a crash in llvm if the sreg of a setret is not set because the methods ends with a throw. (dotnet#49122) [macOS-arm64] Disable failing libraries tests (dotnet#49400) improve PriorityQueue documentation (dotnet#49392) [wasm] Fix debugger tests (dotnet#49206) [mono] Fix the emission of EnumEqualityComparer instances into the corlib AOT image. (dotnet#49402) jitutils M2M renaming reaction (dotnet#49430) WinHttpHandler: Read HTTP/2 trailing headers [RyuJIT] Make casthelpers cold for sealed classes (dotnet#49295) JIT: Non-void ThrowHelpers (dotnet#48589) Update package index for servicing (dotnet#49417) Remove unnecessary check on polymorphic serialization (dotnet#48464) Remove release build cron triggers from jitstress jobs (dotnet#49333) [main] Update dependencies from dotnet/arcade dotnet/llvm-project dotnet/runtime-assets (dotnet#49359) Implement AppleCryptoNative_X509GetRawData using SecCertificateCopyData [AndroidCrypto] Support a zero-length salt for HMACs. (dotnet#49384) Use managed implementation of pbkdf2 for Android's one-shot implementation. (dotnet#49314) Make 303 redirects do GET like Net Framework (dotnet#49095) Make sure event generation is incremental (dotnet#48903) Add amd and Surface arm64 perf runs (dotnet#49389) Enregister EH var that are single def (dotnet#47307) ...
Currently we optimize such casts in JIT to the following:
In case of "sealed" classes that
CORINFO_HELP_CHKCASTCLASS_SPECIAL
is only used to throwInvalidCastException
for cases when the input object is of a wrong type, so we can mark the whole block as rarely-executed.Jit-diff:
Size improvement examples:
https://www.diffchecker.com/4hK014gu
https://www.diffchecker.com/tWgKrbyl (from my understanding - it merged cold tails)
but mostly it's a regression due to additional jumps (from cold blocks).
/cc @AndyAyersMS @VSadov