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

Unnecessary zeroing of boxed default instances #101544

Open
MichalStrehovsky opened this issue Apr 25, 2024 · 4 comments
Open

Unnecessary zeroing of boxed default instances #101544

MichalStrehovsky opened this issue Apr 25, 2024 · 4 comments
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@MichalStrehovsky
Copy link
Member

MichalStrehovsky commented Apr 25, 2024

using System;
using System.Collections.Generic;
public static class C {
    public static object M() {
        return default(KeyValuePair<String, String>);
    }
}

M gets compiled into:

; Assembly listing for method C:M():System.Object (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data

G_M000_IG01:                ;; offset=0x0000
       sub      rsp, 40

G_M000_IG02:                ;; offset=0x0004
       mov      rcx, 0x7FFD31850E58
       call     CORINFO_HELP_NEWSFAST
       xor      ecx, ecx
       mov      qword ptr [rax+0x08], rcx
       mov      qword ptr [rax+0x10], rcx

G_M000_IG03:                ;; offset=0x001D
       add      rsp, 40
       ret

We optimized the box into a sequence of NEWSFAST+initialize fields, realized we're doing zero init (initialization is done with a mov instead of a write barrier), but failed to realize NEWSFAST already returns a zero-initialized thing and initializing fields to zero is unnecessary.

@MichalStrehovsky MichalStrehovsky added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 25, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Apr 25, 2024
Copy link
Contributor

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

@EgorBo
Copy link
Member

EgorBo commented Apr 25, 2024

There is even a more trivial example:

object Foo() => 0; // boxing zero

asm:

; Method MyClass:Foo():System.Object (FullOpts)
       sub      rsp, 40
       mov      rcx, 0x7FFB4FC6F900
       call     CORINFO_HELP_NEWSFAST
       xor      ecx, ecx
       mov      dword ptr [rax+0x08], ecx ;;; <--- redundant
       add      rsp, 40
       ret      
; Total bytes of code: 29

I presume this can be trivially fixed, but to make it more robust and handle a lot more cases we need to model memory in SSA/VN

https://llvm.org/docs/MemorySSA.html

@EgorBo EgorBo removed the untriaged New issue has not been triaged by the area owner label Apr 25, 2024
@EgorBo EgorBo added this to the Future milestone Apr 25, 2024
@AndyAyersMS
Copy link
Member

Seems possibly related to not using field seqs for the stores to boxes; if we did this I think VN's underlying memory model might "just work" and these would get dead stored. See eg #85570.

@jakobbotsch
Copy link
Member

Seems possibly related to not using field seqs for the stores to boxes; if we did this I think VN's underlying memory model might "just work" and these would get dead stored.

#96942 tracks this -- when I looked it's not all that simple to model that "all these fields are zero" like we would need here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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.

4 participants