GT_STORE_BLK - do not call memset for blocks containg gc pointers on heap

[EgorBo]

This PR applies fix suggested by @jkotas to always inline "memset" for structs with GC references since JIT_Memset doesn't provide us with needed atomicity guarantees. An alternative solution is to introduce a new memset helper (it requires R2R format update).

1. Current PR still allows Memset when target is a local (= not visible) - for that we're also allowed to use SIMD (current Main behaviour) - is it correct?
2. It seems that we should allow SIMD as well if we manually align it to 16 bytes (and put it in nongc zone) to optimize the long sequence of 8-byte moves for large structs

void Test(ref LargeStructWithGC s)
{
    s = default;
}

struct LargeStructWithGC // 184 bytes
{
    long b, c, d, e, f, g, h, i, j, k, l, m, n, o, p, r, s, t, u, v, w, z;
    string a;
}

Was:

; Method Program:Test(byref):this (FullOpts)
       sub      rsp, 40
       mov      rcx, rdx
       xor      edx, edx
       mov      r8d, 184
       call     CORINFO_HELP_MEMSET
       nop      
       add      rsp, 40
       ret      
; Total bytes of code: 26

Now:

; Method StructWithGC_Zeroing:Test(byref):this (FullOpts)
       xor      eax, eax
       mov      qword ptr [rdx], rax
       mov      ecx, 176 ;;; 184 - 8
G_M2317_IG03:
       mov      qword ptr [rdx+rcx], rax
       sub      rcx, 8
       jne      SHORT G_M2317_IG03
       ret      
; Total bytes of code: 21

[ghost]

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

Issue Details

---

ci test

Author:	EgorBo
Assignees:	EgorBo
Labels:	area-CodeGen-coreclr
Milestone:	-

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[jkotas]

Current PR still allows Memset when target is a local (= not visible) - for that we're also allowed to use SIMD (current Main behaviour) - is it correct?

Yes, that sounds right.

[jakobbotsch]

I think some fallback for large structs will be needed, either as an inline loop or helper callback, otherwise we can hit encoder limitations. One of the first lessons I learned working on the JIT in #61521.

[EgorBo]

I think some fallback for large structs will be needed, either as an inline loop or helper callback, otherwise we can hit encoder limitations. One of the first lessons I learned working on the JIT in #61521.

Yeah I already hit it locally (added more tests)

[EgorBo]

For reference, an alternative fix to introduce a new helper (with r2r format bump): EgorBo@262c43c

[EgorBo]

Ugh...

[jkotas]

It is why we have ZeroMemoryInGCHeap

[EgorBo]

PTAL @dotnet/jit-contrib @jkotas

I implemented the loop (for cases when size is too big to be unrolled). Diffs are not too big and don't touch sensitive stuff it seems (except, maybe ValueTasks with large state). The loop can be additionally optimized further if needed.

An alternative solution is to introduce a new helper that will ZeroMemoryInGCHeap (something like EgorBo@262c43c + needs a few more changes for X86_32) - but that requires R2R format bump that might not be desired for backports to 8.0 (and 6.0?)

TODO: finish risc-v/la64, just wanted to get a feedback first.

[EgorBo]

/azp run runtime-coreclr outerloop, runtime-coreclr jitstress, Fuzzlyn

[azure-pipelines]

Azure Pipelines successfully started running 3 pipeline(s).

[BruceForstall]

You should add a section to https://github.com/dotnet/runtime/blob/main/docs/design/coreclr/botr/clr-abi.md detailing the GC requirements for memset/memcpy atomicity.

[EgorBo]

Another problem we discovered (with @MichalPetryka) is that RISC-V (and LA64?) don't have addressing modes for indices (base + index), only for constant imms. It means we need to prepare the address in a separate temp reg so I should add it to gcinfo or mark loop as nogc I assume.

[am11]

RISC-V (and LA64?) don't have addressing modes for indices (base + index)

Yup, emitInsLoadStoreOp in emit{loong,riscv}64.cpp also handle them as complex addressing modes.

[clamp03]

LGTM Thank you @EgorBo
cc @tomeksowi

[tomeksowi]

RISC-V part LGTM, thx @EgorBo

[EgorBo]

@BruceForstall can you please review & approve if it's fine?

Diffs

Jitstress and gcstress passed

[BruceForstall]

LGTM. Minor notes on comments.

[EgorBo]

Since the last approval the only changes were in RISC/LA to address feedback from RISC/LA teams.

[EgorBo]

@jkotas does this need to be backported to any release?

Presumably the actual bug was there since .NET Framework?

[jkotas]

Presumably the actual bug was there since .NET Framework?

.NET Framework has a different (slower) implementation of JIT_MemSet that does not appear to have the tearing problem. It does not use rep stosb.

does this need to be backported to any release?

It was found via internal escalation (service crashing 3x per day). We should backport it to .NET 8.

(It was hit on .NET 6, but the affected customer is planning to move to .NET 8 soon. .NET 8 backport should be good enough.)

[EgorBo]

/backport to release/8.0-staging

[github-actions]

Started backporting to release/8.0-staging: https://github.com/dotnet/runtime/actions/runs/7416001475