- Notifications
You must be signed in to change notification settings - Fork5.2k
Remove boxing from "value is S" for nullables#95711
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
ghost commentedDec 7, 2023
Tagging subscribers to this area:@JulieLeeMSFT,@jakobbotsch Issue DetailsCloses#95685 It turns out to be a simple fix boolTest1<T>(Tvalue)// T is int?{returnvalueisint;}voidTest2<T>(Tvalue)// T is int?{if(valueisint)Console.WriteLine();} ; Assembly listing for method Prog:Test1[System.Nullable`1[int]]- sub rsp, 40- mov qword ptr [rsp+0x38], rdx- lea rdx, [rsp+0x38]- mov rcx, 0xD1FFAB1E ; System.Nullable`1[int]- call CORINFO_HELP_BOX_NULLABLE- test rax, rax- je SHORT G_M33107_IG04- mov rcx, 0xD1FFAB1E ; System.Int32- xor rdx, rdx- cmp qword ptr [rax], rcx- cmovne rax, rdx-G_M33107_IG04:- test rax, rax- setne al- movzx rax, al- add rsp, 40+ xor eax, eax ret-; Total bytes of code 67+; Total bytes of code 3; Assembly listing for method Prog:Test2[System.Nullable`1[int]]- sub rsp, 40- mov qword ptr [rsp+0x38], rdx- cmp byte ptr [rsp+0x38], 0- je SHORT G_M14037_IG04- call [System.Console:WriteLine()]-G_M14037_IG04:- nop- add rsp, 40 ret-; Total bytes of code 28+; Total bytes of code 1
|
EgorBo commentedDec 7, 2023
| case CEE_LDNULL: | ||
| { | ||
| // "ldnull + cgt_un" is used when BOX+ISINST is not fed into a branch, e.g.: | ||
| // | ||
| // if (obj is string) { <--- box + isinst + br_true | ||
| // | ||
| // bool b = obj is string; <--- box + isinst + ldnull + cgt_un | ||
| // | ||
| // bool b = obj is not string; <--- box + isinst + ldnull + cgt_un + ldc.i4.0 + ceq | ||
| // |
huoyaoyuanDec 7, 2023 • edited
Loading Uh oh!
There was an error while loading.Please reload this page.
edited
Uh oh!
There was an error while loading.Please reload this page.
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 did similar transform in#47920 (comment) . It's interesting to see which implementation covers more cases.
I'll push it later today.
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 think I tried this transformation I while back and gave up due to small diffs, decided to push it because of customer reported issue
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.
main...huoyaoyuan:runtime:isinst-null-cmp
It doesn't help, and doesn't conflict either. Let's do separately.
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 think my change is simpler? (if you ignore whitespaces).
Also, not sure ifbox + isinst + ldnull + ceq ever shows up - Roslyn seems to not emitting it so I didn't bother
E.g.sharplab
EgorBo commentedDec 7, 2023
PTAL @dotnet/jit-contrib,@jakobbotsch A small change (if you disable whitespaces) that closes#95685 Almost no hits in BCL (except afew tests in minopts) |
Uh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
Closes#95685
I recommend reviewing with "hide whitespaces" mode on the review tab. I removed some codeAddr validations and replaced those with
impGetNonPrefixOpcodethat does the same.