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

Question: Is coreclr's use of SIMD-accelerated initobj legal for ref types T? #51638

Closed
GrabYourPitchforks opened this issue Apr 21, 2021 · 4 comments · Fixed by #53116
Closed
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI memory model issues associated with memory model question Answer questions and provide assistance, not an issue with source code or documentation.
Milestone

Comments

@GrabYourPitchforks
Copy link
Member

This is extracted out of a discussion whose salient points are #51534 (comment) and #51534 (comment).

In a nutshell, when the JIT sees an initobj instruction, it will attempt to perform SIMD writes to the destination address, This is true whether or not the target type contains gcrefs. See the below example for a demonstration.

using System;

public class C {
    internal struct MyStruct
    {
        object _data0;
        object _data1;
    }
    
    internal class MyClass1
    {
        MyStruct _s;
        
        void Clear()
        {
            _s = default;
        }
    }
    
    internal class MyClass2
    {
        object _dataX;
        MyStruct _s;
        
        void Clear()
        {
            _s = default;
        }
    }
}
; Core CLR v5.0.421.11614 on amd64

MyClass1.Clear()
    L0000: vzeroupper
    L0003: vxorps xmm0, xmm0, xmm0
    L0007: vmovdqu [rcx+8], xmm0
    L000c: ret

MyClass2.Clear()
    L0000: vzeroupper
    L0003: vxorps xmm0, xmm0, xmm0
    L0007: vmovdqu [rcx+0x10], xmm0
    L000c: ret

In this example, references to MyStruct are only guaranteed 8-byte aligned, not 16-byte aligned. So it's possible that the vmovdqu instruction could straddle a cache line boundary or a page boundary. The implicit assumption here is that even if the SIMD write as a whole is not atomic, each individual native word-sized element of the vector is published atomically (since the target address is native word-aligned).

Per #51534 (comment), this assumption is valid for ARM64. But per #51534 (comment), this assumption might not be valid for x86 / x64 / ARM32.

So, the larger question: is the current JIT behavior legal such that no other thread will ever see a torn write to a gcref field, or is the JIT relying on undocumented behavior? And if we're relying on a potentially invalid assumption, do we need to change our behavior?

@GrabYourPitchforks GrabYourPitchforks added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 21, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Apr 21, 2021
@GrabYourPitchforks
Copy link
Member Author

/cc @dotnet/jit-contrib

@tannergooding
Copy link
Member

Notably Intel documents the following in "Volume 3A. 8.1.1 Guaranteed Atomic Operations"

image

AMD likewise documents in "Volume 1. 3.9.1.3 Atomicity of accesses"
image

@JulieLeeMSFT JulieLeeMSFT added the question Answer questions and provide assistance, not an issue with source code or documentation. label Apr 21, 2021
@JulieLeeMSFT
Copy link
Member

@echesakovMSFT PTAL.

@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone May 10, 2021
@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label May 10, 2021
@echesakov
Copy link
Contributor

I will investigate

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label May 22, 2021
echesakov added a commit that referenced this issue Jun 15, 2021
… stores (x86)/ two 8-byte mov-s (x64) (#53116)

Fixes #51638 by using

1) Constructing `ASG(OBJ(addr), 0)` for structs that have GC fields and keeping the current IR (i.e. `ASG(BLK(addr), 0)`) for other types. Such bookkeeping would allow the JIT to maintain information about the class layout.

2a) Emitting a sequence of `mov [m64],r64` instead of `movdqu [m128],xmm` when zeroing structs with GC fields that are not guaranteed to be on the stack on win-x64 or linux-x64.

2b) Emitting a sequence of `movq [m64],xmm` when zeroing such structs on win-x86.
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Jun 15, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jul 15, 2021
@jkotas jkotas added the memory model issues associated with memory model label Oct 2, 2022
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 memory model issues associated with memory model question Answer questions and provide assistance, not an issue with source code or documentation.
Projects
None yet
5 participants