Skip to content
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

Remove boxing from "value is S" for nullables #95711

Merged
merged 6 commits into from
Dec 7, 2023

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Dec 7, 2023

Closes #95685

bool Test<T>(T value) => value is int; // T is Nullable<int>
; Assembly listing for method Prog:Test[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
+      mov      qword ptr [rsp+0x10], rdx
+      movzx    rax, byte  ptr [rsp+0x10]
       ret      
-; Total bytes of code 67
+; Total bytes of code 11

I recommend reviewing with "hide whitespaces" mode on the review tab. I removed some codeAddr validations and replaced those with impGetNonPrefixOpcode that does the same.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Dec 7, 2023
@ghost ghost assigned EgorBo Dec 7, 2023
@ghost
Copy link

ghost commented Dec 7, 2023

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

Closes #95685

It turns out to be a simple fix
Example:

bool Test1<T>(T value) // T is int?
{
    return value is int;
}

void Test2<T>(T value) // T is int?
{
    if (value is int)
        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
Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo EgorBo force-pushed the box-isinst-nullable branch from 362da42 to f0c4fff Compare December 7, 2023 02:52
@EgorBo
Copy link
Member Author

EgorBo commented Dec 7, 2023

@MihuBot

Comment on lines +2966 to +2975
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
//
Copy link
Member

@huoyaoyuan huoyaoyuan Dec 7, 2023

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.

Copy link
Member Author

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

Copy link
Member

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.

Copy link
Member Author

@EgorBo EgorBo Dec 7, 2023

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 if box + isinst + ldnull + ceq ever shows up - Roslyn seems to not emitting it so I didn't bother

E.g. sharplab

@EgorBo EgorBo marked this pull request as ready for review December 7, 2023 09:13
@EgorBo
Copy link
Member Author

EgorBo commented Dec 7, 2023

PTAL @dotnet/jit-contrib, @jakobbotsch A small change (if you disable whitespaces) that closes #95685

Almost no hits in BCL (except a few tests in minopts)

@EgorBo EgorBo merged commit 8383c97 into dotnet:main Dec 7, 2023
129 checks passed
@EgorBo EgorBo deleted the box-isinst-nullable branch December 7, 2023 13:26
@github-actions github-actions bot locked and limited conversation to collaborators Jan 7, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generic type check allocates for nullable structs
3 participants