JIT: Writing into local memory prevents hoisting loads out of loop

[BoyBaykiller]

https://godbolt.org/z/f7xheT6E6

Inside the loop I expect 1 load per iteration from buildData.OriginalTriIds.
The actual number of loads is 4. It seems like loop hoisting gives up because of the write into indices. But indices shouldn't be able to alias leafNode in any way:

static Span<int> GetUnindexed(in GpuBlasNode leafNode, BuildData buildData)
{
    Span<int> indices = new int[leafNode.TriCount];
    for (int i = 0; i < leafNode.TriCount; i++)
    {
        indices[i] = buildData.OriginalTriIds[leafNode.TriStartOrChild + i];
    }

    return indices;
}

leafNode.TriCount, leafNode.TriStartOrChild and buildData.OriginalTriIds.Length (for the bounds check) can be hoisted.

Program:GetUnindexed(byref,Program+BuildData):System.Span`1[int] (FullOpts):
       push     rbp
       push     r15
       push     r14
       push     rbx
       push     rax
       lea      rbp, [rsp+0x20]
       mov      rbx, rdi
       mov      r15, rsi
       mov      r14d, dword ptr [rbx+0x24]
       movsxd   rsi, r14d
       mov      rdi, 0x7AD3E913A7C0      ; int[]
       call     CORINFO_HELP_NEWARR_1_VC
       add      rax, 16
       mov      edx, r14d
       xor      ecx, ecx
       test     r14d, r14d
       jle      SHORT G_M9601_IG04
       align    [10 bytes for IG03]
G_M9601_IG03:  ;; offset=0x0040
       cmp      ecx, edx
       jae      SHORT G_M9601_IG05
       mov      edi, ecx
       add      edi, dword ptr [rbx+0x10] ; <-- load leafNode.TriStartOrChild
       cmp      edi, dword ptr [r15+0x08] ; <-- load buildData.OriginalTriIds.Length
       jae      SHORT G_M9601_IG05
       mov      edi, dword ptr [r15+4*rdi+0x10]
       mov      dword ptr [rax+4*rcx], edi
       inc      ecx
       cmp      ecx, dword ptr [rbx+0x24] ; <-- load leafNode.TriCount
       jl       SHORT G_M9601_IG03
G_M9601_IG04:  ;; offset=0x005E
       add      rsp, 8
       pop      rbx
       pop      r14
       pop      r15
       pop      rbp
       ret      
G_M9601_IG05:  ;; offset=0x0069
       call     CORINFO_HELP_RNGCHKFAIL
       int3     

[dotnet-policy-service]

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

[AndyAyersMS]

Because of the in, leafNode is passed by reference, and the JIT has to assume the backing storage might be on the heap, and so its fields might be concurrently modified by some other thread, so it is not safe to hoist those loads. You would have to manually hoist them to locals to convey intent.

Removing the in improves codegen for the loop, the struct fields now can't be concurrently modified.

Making GpuBlasNode a ref struct should have similar effect, but it doesn't...

[BoyBaykiller]

Because of the in, leafNode is passed by reference, and the JIT has to assume the backing storage might be on the heap, and so its fields might be concurrently modified by some other thread, so it is not safe to hoist those loads

But then why does it hoist in other cases like:

// https://godbolt.org/z/v48ov9cqj
static int ByRef(in GpuBlasNode leafNode)
{
    int sum = 0;
    for (int i = 0; i < leafNode.TriCount; i++)
    {
        sum += i;
    }

    return sum;
}

       mov      edx, dword ptr [rdi+0x24] ; load leafNode.TriCount outside the loop
G_M61276_IG03:  ;; offset=0x000F
       add      eax, ecx
       inc      ecx
       cmp      ecx, edx
       jl       SHORT G_M61276_IG03

As we can see leafNode.TriCount is loaded into a register before the loop starts. leafNode.TriCount might be modified by an other thread while we are inside the loop, so as far I understand you, this shouldn't be allowed either as it can lead to an observable difference.

[BoyBaykiller]

Don't we have Volatile.Read to prevent hoisting here. As in, without it JIT should be allowed to.

[AndyAyersMS]

In cases that won't compromise type safety and where the underlying behavior involves a race condition, the jit can be more aggressive.

See the Side-effects and optimizations of memory accesses in the memory model doc.

[BoyBaykiller]

How exactly does ByRef satisfy "won't compromise type safety and where the underlying behavior involves a race condition", but my original function GetUnindexed does not? I really want to understand this, so in the future I can tell whats a JIT limitation and what simply isn't allowed. I've tried reading the memory model doc to answer this question.

The way I see it: 99.9% of time functions are not written with the intention to have changes be made to the arguments from other threads during execution, so the memory model should allow them to be hoisted (like in C++). For the 0.1% where this is the case explicit use of Volatile can be used to convey intention and tell the compiler not to hoist/optimize

[AndyAyersMS]

The way I see it: 99.9% of time functions are not written with the intention to have changes be made to the arguments from other threads during execution, so the memory model should allow them to be hoisted (like in C++). For the 0.1% where this is the case explicit use of Volatile can be used to convey intention and tell the compiler not to hoist/optimize

Unfortunately we generally can't make optimistic assumptions like this if they can compromise type or memory safety. Instead we have to be pessimistic.

So if you want the JIT to know that values that might be on the heap cannot change asynchronously you need to manually load those into locals. That also helps fix order of possible exception side effects.

[BoyBaykiller]

Unfortunately we generally can't make optimistic assumptions like this if they can compromise type or memory safety. Instead we have to be pessimistic.

How could hoisting compromise type or memory safety in GetUnindexed?

I understood that the memory model is not as permissive as I thought. I am still trying to understand why hoisting is nonetheless
allowed in some cases (ByRef) and not in others (GetUnindexed) - in the presence of threads modifying the arguments. Thanks!

[AndyAyersMS]

I'd have to go dig into how the JIT is actually modelling things here. It's possible the JIT is being too pessimistic or that the concern is for altering exceptional behavior. But the broader point stands, if you know some possibly-on-the-heap values are invariant during some computation, manually caching them in locals (or passing them in by-value) will generally give better codegen.

[BoyBaykiller]

Alright. The reason I tend to use pass by-reference rather than pass by-value is to avoid the copy cost. Unaware that hoisting is not as aggressive as I might have guessed, such that it easily ends up being a de-optimization. I guess I'll have to reevaluate that or start hoisting manually, but I am also hoping for the JIT to be as aggressive as possible (within the memory model).

Anyway, if it turns out the memory model doesn't allow it in this case, then feel free to close as not planned.

PS: I would like to note a previous question I asked on this topic and it's answers: https://discord.com/channels/143867839282020352/312132327348240384/1335362136750428275