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

Profiled casts: isinst is no longer hoisted #96837

Closed
EgorBo opened this issue Jan 11, 2024 · 6 comments · Fixed by #97075
Closed

Profiled casts: isinst is no longer hoisted #96837

EgorBo opened this issue Jan 11, 2024 · 6 comments · Fixed by #97075
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@EgorBo
Copy link
Member

EgorBo commented Jan 11, 2024

Since we expand a lot more casts now, we hit more cases where expanded (early) cast becomes non-hoistable, example:

[MethodImpl(MethodImplOptions.NoInlining)]
bool foo(object o)
{
    bool result = false;
    for (int i = 0; i < 100000; i++)
        result = o is IDisposable;  // o is "mostly" Program instance in my benchmark
    return result;
}

Codegen with DOTNET_JitProfileCasts=0:

G_M31828_IG01:
       push     rbx
       sub      rsp, 32
G_M31828_IG02:
       xor      ebx, ebx
       mov      rcx, 0x7FF80DB09C28      ; System.IDisposable
       call     CORINFO_HELP_ISINSTANCEOFINTERFACE
       test     rax, rax
       setne    al
       movzx    rax, al
       jmp      SHORT G_M31828_IG03
       align    [0 bytes for IG03]

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; <loop_start>
G_M31828_IG03:
       mov      ecx, eax
       inc      ebx
       cmp      ebx, 0x186A0
       jl       SHORT G_M31828_IG03
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; <loop_end>

G_M31828_IG04:
       mov      eax, ecx
G_M31828_IG05:
       add      rsp, 32
       pop      rbx
       ret

Codegen with DOTNET_JitProfileCasts=1:

G_M31828_IG01:
       push     rsi
       push     rbx
       sub      rsp, 40
       mov      rbx, rdx
G_M31828_IG02:
       xor      esi, esi

;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; <loop_start>
G_M31828_IG03:
       mov      rax, rbx
       test     rax, rax
       je       SHORT G_M31828_IG06
G_M31828_IG04:
       mov      rdx, 0x7FF80DDE0988      ; Program
       cmp      qword ptr [rax], rdx
       je       SHORT G_M31828_IG06
G_M31828_IG05:
       mov      rdx, rbx
       mov      rcx, 0x7FF80DAF9C28      ; System.IDisposable
       call     CORINFO_HELP_ISINSTANCEOFINTERFACE
G_M31828_IG06:
       test     rax, rax
       setne    al
       movzx    rax, al
       inc      esi
       cmp      esi, 0x186A0
       jl       SHORT G_M31828_IG03
;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;;; <loop_end>

G_M31828_IG07:
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret

Options:

  1. Don't instrument isinst if it's inisde a loop (backward branch) - the easiest and the most conservative option.
  2. Expand all casts in a late phase - we might miss some branch opts
  3. Extend loop clonning

castclass is fine, we don't hoist it due to side-effects anyway.

@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 11, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 11, 2024
@ghost
Copy link

ghost commented Jan 11, 2024

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

Issue Details

Since we expand a lot more casts now, we hit more cases where expanded (early) cast becomes non-hoistable, example:

[MethodImpl(MethodImplOptions.NoInlining)]
bool foo(object o)
{
    bool result = false;
    for (int i = 0; i < 100000; i++)
        result = o is IDisposable;  // o is "mostly" Program instance in my benchmark
    return result;
}

Codegen with DOTNET_JitProfileCasts=0:

G_M31828_IG01:  ;; offset=0x0000
       push     rbx
       sub      rsp, 32
G_M31828_IG02:  ;; offset=0x0005
       xor      ebx, ebx
       mov      rcx, 0x7FF80DB09C28      ; System.IDisposable
       call     CORINFO_HELP_ISINSTANCEOFINTERFACE
       test     rax, rax
       setne    al
       movzx    rax, al
       jmp      SHORT G_M31828_IG03
       align    [0 bytes for IG03]
G_M31828_IG03:  ;; offset=0x0021
       mov      ecx, eax
       inc      ebx
       cmp      ebx, 0x186A0
       jl       SHORT G_M31828_IG03
G_M31828_IG04:  ;; offset=0x002D
       mov      eax, ecx
G_M31828_IG05:  ;; offset=0x002F
       add      rsp, 32
       pop      rbx
       ret

Codegen with DOTNET_JitProfileCasts=1:

G_M31828_IG01:  ;; offset=0x0000
       push     rsi
       push     rbx
       sub      rsp, 40
       mov      rbx, rdx
G_M31828_IG02:  ;; offset=0x0009
       xor      esi, esi
G_M31828_IG03:  ;; offset=0x000B
       mov      rax, rbx
       test     rax, rax
       je       SHORT G_M31828_IG06
G_M31828_IG04:  ;; offset=0x0013
       mov      rdx, 0x7FF80DDE0988      ; Program
       cmp      qword ptr [rax], rdx
       je       SHORT G_M31828_IG06
G_M31828_IG05:  ;; offset=0x0022
       mov      rdx, rbx
       mov      rcx, 0x7FF80DAF9C28      ; System.IDisposable
       call     CORINFO_HELP_ISINSTANCEOFINTERFACE
G_M31828_IG06:  ;; offset=0x0034
       test     rax, rax
       setne    al
       movzx    rax, al
       inc      esi
       cmp      esi, 0x186A0
       jl       SHORT G_M31828_IG03
G_M31828_IG07:  ;; offset=0x0047
       add      rsp, 40
       pop      rbx
       pop      rsi
       ret
Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@jakobbotsch
Copy link
Member

  1. Extend loop clonning

Do you mean hoisting?

@jakobbotsch
Copy link
Member

Is CSE able to clean it up if you peel one iteration of the loop manually?

@EgorBo
Copy link
Member Author

EgorBo commented Jan 11, 2024

  1. Extend loop clonning

Do you mean hoisting?

No, clonning

Is CSE able to clean it up if you peel one iteration of the loop manually?

Nope 😢 or I am doing it wrong

@jakobbotsch
Copy link
Member

jakobbotsch commented Jan 11, 2024

Ah yeah, I guess the extra control flow makes us unable to handle it by CSE and makes it not really feasible to improve hoisting to handle it too, hmmpf. This is sort of the "hammock" concept that @AndyAyersMS talks about sometimes, I think, where multiple blocks of the flow graph can/should be considered as one atomic thing.

@JulieLeeMSFT JulieLeeMSFT added this to the 9.0.0 milestone Jan 13, 2024
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Jan 13, 2024
@jakobbotsch jakobbotsch assigned EgorBo and unassigned jakobbotsch Jan 13, 2024
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Jan 17, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jan 19, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 3, 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 a pull request may close this issue.

3 participants