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

Reduce size of ArgumentOutOfRangeException throw helpers when not inlined #88508

Merged
merged 1 commit into from
Jul 7, 2023

Conversation

stephentoub
Copy link
Member

Keeping the arguments of the delegated throw method the same order as the callee avoids unnecessary spilling. Per #79157 (comment).

e.g. ThrowIfGreaterThan<int> before:

    L0000: sub rsp, 0x28	
    L0004: cmp ecx, edx	
    L0006: jl short L0014	
    L0008: mov [rsp+0x30], ecx	
    L000c: mov [rsp+0x38], edx	
    L0010: cmp ecx, edx	
    L0012: jg short L0019	
    L0014: add rsp, 0x28	
    L0018: ret	
    L0019: mov rcx, r8	
    L001c: mov edx, [rsp+0x30]	
    L0020: mov r8d, [rsp+0x38]	
    L0025: call 0x00007ffdfec90108	
    L002a: int3

and after:

    L0000: sub rsp, 0x28
    L0004: cmp ecx, edx
    L0006: jl short L000c
    L0008: cmp ecx, edx
    L000a: jg short L0011
    L000c: add rsp, 0x28
    L0010: ret
    L0011: call 0x00007ffdfec90150
    L0016: int3

cc: @AndyAyersMS

Keeping the arguments of the delegated throw method the same order as the callee avoids unnecessary spilling.
@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 7, 2023
@ghost ghost assigned stephentoub Jul 7, 2023
@stephentoub stephentoub added area-System.Runtime tenet-performance Performance related issue and removed needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 7, 2023
@ghost
Copy link

ghost commented Jul 7, 2023

Tagging subscribers to this area: @dotnet/area-system-runtime
See info in area-owners.md if you want to be subscribed.

Issue Details

Keeping the arguments of the delegated throw method the same order as the callee avoids unnecessary spilling. Per #79157 (comment).

e.g. ThrowIfGreaterThan<int> before:

    L0000: sub rsp, 0x28	
    L0004: cmp ecx, edx	
    L0006: jl short L0014	
    L0008: mov [rsp+0x30], ecx	
    L000c: mov [rsp+0x38], edx	
    L0010: cmp ecx, edx	
    L0012: jg short L0019	
    L0014: add rsp, 0x28	
    L0018: ret	
    L0019: mov rcx, r8	
    L001c: mov edx, [rsp+0x30]	
    L0020: mov r8d, [rsp+0x38]	
    L0025: call 0x00007ffdfec90108	
    L002a: int3

and after:

    L0000: sub rsp, 0x28
    L0004: cmp ecx, edx
    L0006: jl short L000c
    L0008: cmp ecx, edx
    L000a: jg short L0011
    L000c: add rsp, 0x28
    L0010: ret
    L0011: call 0x00007ffdfec90150
    L0016: int3

cc: @AndyAyersMS

Author: stephentoub
Assignees: stephentoub
Labels:

area-System.Runtime, tenet-performance

Milestone: -

@stephentoub stephentoub merged commit 996b012 into dotnet:main Jul 7, 2023
@stephentoub stephentoub deleted the throwhelpersize branch July 7, 2023 14:56
@runfoapp runfoapp bot mentioned this pull request Jul 7, 2023
@AndyAyersMS
Copy link
Member

Thanks... I have local changes to fix this "redundant" compare but they are a bit complex and fire so infrequently that I am wavering on whether or not to add this.

    L0000: sub rsp, 0x28
    L0004: cmp ecx, edx
    L0006: jl short L000c
    L0008: cmp ecx, edx
    L000a: jg short L0011
    L000c: add rsp, 0x28
    L0010: ret
    L0011: call 0x00007ffdfec90150
    L0016: int3

could be

    L0000: sub rsp, 0x28
    L0004: cmp ecx, edx
    L0006: jg short Lxxxx
           add rsp, 0x28
           ret
    Lxxxx: call 0x00007ffdfec90150
           int3

(and if we ever really get clever)

          cmp ecx, edx
          jg short Lxxxx
          ret
   Lxxxx: sub rsp, 0x28
          call 0x00007ffdfec90150
          int3

@ghost ghost locked as resolved and limited conversation to collaborators Aug 6, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants