Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # 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
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants