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

JIT: Avoid boxing ArgumentNullException.ThrowIfNull arguments in Tier-0 #104815

Merged
merged 8 commits into from
Jul 15, 2024

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Jul 12, 2024

Closes #104512

Example:

static void Test<T>(T a)
{
    ArgumentNullException.ThrowIfNull(a);
}

Current codegen for Test<int> in Tier0:

; Assembly listing for method Program:Test[int](int) (Tier0)
; Tier0 code
       push     rbp
       sub      rsp, 48
       lea      rbp, [rsp+0x30]
       xor      eax, eax
       mov      qword ptr [rbp-0x08], rax
       mov      dword ptr [rbp+0x10], ecx
       mov      rcx, 0x7FFD546561B0      ; System.Int32
       call     CORINFO_HELP_NEWSFAST
       mov      gword ptr [rbp-0x08], rax
       mov      rax, gword ptr [rbp-0x08]
       mov      ecx, dword ptr [rbp+0x10]
       mov      dword ptr [rax+0x08], ecx
       mov      rcx, gword ptr [rbp-0x08]
       mov      rdx, 0x203802095B0      ; 'a'
       call     [System.ArgumentNullException:ThrowIfNull(System.Object,System.String)]
       nop      
       add      rsp, 48
       pop      rbp
       ret     

New codegen for Test<int> in Tier0:

; Assembly listing for method Program:Test[int](int) (Tier0)
; Tier0 code
       push     rbp
       sub      rsp, 16
       lea      rbp, [rsp+0x10]
       xor      eax, eax
       mov      qword ptr [rbp-0x08], rax
       mov      dword ptr [rbp+0x10], ecx
       add      rsp, 16
       pop      rbp
       ret

@AndyAyersMS I hope I didn't step on your foot since it's was assigned to you, it's just that it looked trivial and I decided to file a quick fix 🙂

In theory, we could solve this by a limitted version of inlining in tier0, but that is tricky and may regress startup time/lead to unnecessary type load events, etc.

@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 Jul 12, 2024
Copy link
Contributor

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

Copy link
Member

@AndyAyersMS AndyAyersMS left a comment

Choose a reason for hiding this comment

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

I was thinking we'd catch this in impBoxPatternMatch to avoid creating and then removing all the BOX IR, but that would require recognizing and intrinsic from its IL, which I guess could be clunky. This works too.

@AndyAyersMS
Copy link
Member

@AndyAyersMS I hope I didn't step on your foot since it's was assigned to you, it's just that it looked trivial and I decided to file a quick fix 🙂

No worries, I hadn't gotten around to this yet.

@EgorBo
Copy link
Member Author

EgorBo commented Jul 12, 2024

I was thinking we'd catch this in impBoxPatternMatch to avoid creating and then removing all the BOX IR, but that would require recognizing and intrinsic from its IL, which I guess could be clunky. This works too.

The problem with that, - we'd need to do a redundant "resolveToken" call on call, yeah

@EgorBo
Copy link
Member Author

EgorBo commented Jul 14, 2024

@jakobbotsch @AndyAyersMS could you please review the Nullable<> part?

Co-authored-by: Jakob Botsch Nielsen <[email protected]>
@EgorBo EgorBo merged commit 5130683 into dotnet:main Jul 15, 2024
166 checks passed
@EgorBo EgorBo deleted the fix-alloc-throwifnull branch July 15, 2024 14:38
@github-actions github-actions bot locked and limited conversation to collaborators Aug 15, 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.

Avoid boxing ArgumentNullException.ThrowIfNull arguments in Tier-0
4 participants