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

Intrinsify typeof(T).IsGenericType #97148

Conversation

Sergio0694
Copy link
Contributor

@Sergio0694 Sergio0694 commented Jan 18, 2024

Contributes to #96898

public static bool Test<T>() => typeof(T).IsGenericType;

Codegen diffs:

; Assembly listing for method Methods:Test[int]():ubyte (FullOpts)
-   sub      rsp, 40
-   mov      rcx, 0x1D980105580      ; 'System.Int32'
-   call     [System.RuntimeType:get_IsGenericType():ubyte:this]
-   add      rsp, 40
+   xor      eax, eax
    ret

; Assembly listing for method Methods:Test[System.Nullable`1[int]]():ubyte (FullOpts)
-   sub      rsp, 40
-   mov      rcx, 0x1D980109298 
-   call     [System.RuntimeType:get_IsGenericType():ubyte:this]
-   nop 
-   add      rsp, 40
+   mov      eax, 1
    ret      

; Assembly listing for method Methods:Test[System.Collections.Generic.KeyValuePair`2[System.__Canon,System.__Canon]]():ubyte (FullOpts)
-   sub      rsp, 40
-   mov      qword ptr [rsp+0x20], rcx
-   mov      rcx, qword ptr [rcx+0x38]
-   mov      rcx, qword ptr [rcx]
-   call     CORINFO_HELP_TYPEHANDLE_TO_RUNTIMETYPE
-   mov      rcx, rax
-   call     [System.RuntimeType:get_IsGenericType():ubyte:this]
-   nop      
-   add      rsp, 40
+   mov      eax, 1
    ret

Added tests but not entirely sure how to get diffs too 😅

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Jan 18, 2024
@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 Jan 18, 2024
@ghost
Copy link

ghost commented Jan 18, 2024

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

Issue Details

Contributes to #96898

Added tests but not entirely sure how to get diffs too 😅

Author: Sergio0694
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/is-generic-type-intrinsic branch 2 times, most recently from 527abd9 to fa02580 Compare January 18, 2024 13:58
@Sergio0694
Copy link
Contributor Author

@MihuBot

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/is-generic-type-intrinsic branch from fa02580 to afde0ce Compare January 18, 2024 16:16
@Sergio0694
Copy link
Contributor Author

@MichalStrehovsky just making sure I'm following: so we don't need any NAOT-specific work for this nor for #97159?

@MichalStrehovsky
Copy link
Member

@MichalStrehovsky just making sure I'm following: so we don't need any NAOT-specific work for this nor for #97159?

We shouldn't need it - unless you actually need this optimization to kick in during NAOT whole program analysis (the answer would typically be "I don't need that" if the T you need to optimize is a valuetype, or there's no pathologic genericness that is hard to get rid of once it makes into whole program view behind the if branch)

@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/is-generic-type-intrinsic branch from a12a551 to a5c7e1b Compare January 18, 2024 18:51
@Sergio0694 Sergio0694 force-pushed the user/sergiopedri/is-generic-type-intrinsic branch from a5c7e1b to eb2b3a9 Compare January 18, 2024 18:52
}
else if ((info.compCompHnd->getClassAttribs(hClass) & CORINFO_FLG_SHAREDINST) != 0)
{
// if we have no type arguments, check that the type itself is not __Canon, and
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You will also get here for __Canon[] that it is perfectly fine to hardcode false for.

I understand that you are trying to reuse the existing JIT/EE interface APIs and flags in creative ways, but doing so often leads to bugs like #97134 (comment)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. I assume for the same reason we shouldn't just try to check for well known cases here, like excluding arrays and pointers types? If we should add a new JIT/EE method for this, what other VM method should it be using instead, is there one that already does what we want to do here? 🤔

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, introducing JIT/EE interface method that answers the exact question tends to work the best. It can be similar to the existing isEnum method.

@ghost
Copy link

ghost commented Feb 17, 2024

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

@github-actions github-actions bot locked and limited conversation to collaborators Mar 19, 2024
This pull request was closed.
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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants