Superflous store generated by JIT for embedded ReadOnlySpan<byte>

[damageboy]

When using a static ReadOnlySpan<byte>, such as this:

internal static ReadOnlySpan<byte> Mask0 => new byte[] {
    0x00, 0x00, 0x00, 0x00, // 0 -> 0
    0xFF, 0xFF, 0xFF, 0xFF, // 1 -> -1
    0xFF, 0xFF, 0xFF, 0xFF, // 2 -> -1
    0xFF, 0xFF, 0xFF, 0xFF, // 3 -> -1
    0xFF, 0xFF, 0xFF, 0xFF, // 4 -> -1
    0xFF, 0xFF, 0xFF, 0xFF, // 5 -> -1
    0xFF, 0xFF, 0xFF, 0xFF, // 6 -> -1
    0xFF, 0xFF, 0xFF, 0xFF, // 7 -> -1
};   

The JIT hard-codes the address in the generated code as expected,

However, when using a fixed statement to load this byte array with an AVX intrinsic (for example):

fixed (byte* bp = &Mask0.GetPinnableReference()) {
        mask0 = LoadDquVector256(bp).AsInt32();
}

The JIT generated a superflous store onto the stack for the fixed statement:

L000e: mov rax, 0x27ff5fb0970
L0018: mov [rsp+0x30], rax ; Why?
L001d: vlddqu ymm0, [rax]

Full listing + asm here

category:cq
theme:pinning
skill-level:beginner
cost:small

[mikedn]

L0018: mov [rsp+0x30], rax ; Why?

That's how fixed works. The fixed pointer needs to be stored in a local variable that's reported to the GC such that it knows not to move the object. If you look later in the assembly code you'll also see that 0 gets stored to that variable, that's the end of the fixed block.

Ideally the JIT would figure out that in this very particular case the fixed machinery isn't needed and eliminate it. That's likely not going to be easy though.

[damageboy]

@mikedn I do understand that, and I understand how this is a very particular case.

I just think that adding this ROS optimization into the JIT + Compiler was a lot of work for generating super efficient yet clean looking code, but I can't get the best outcome with x86 intrinsics, given that they mostly accept pointers and nothing else.

I think it could either be solved by detecting that bp is never written to again, and this is effectively a constant, but I'm way out of my depth here.

I can lift the ptr into a general purpose register (as I've done in my real code) and use that variable / register, it just seems like someone should be able to connect the dots here (e.g. the JIT)....

[Suchiman]

Can we detect this with a peephole optimization to propagate the constant address? Similiar to this trick?

[mikedn]

Can we detect this with a peephole optimization to propagate the constant address?

Unlikely. Normally the only way to eliminate a store is to prove that it's dead and that's not something peephole optimizations can do (well, except trivial cases such 2 consecutive stores to the same location). The fact that in this case a constant gets stored into a pinned variable is not sufficient.

And then the problem is that liveness analysis ignores pinned variables due the dependency between such variables and variables that may contain pointers derived from the original fixed pointer. That is, it's not sufficient to prove that the pinned variable is dead, you have to prove that all other variables containing derived pointers are dead too.

[Suchiman]

If one presumes that the ROS optimization is performed and then sprinkles a bit of Unsafe around, you can shave off 5 instructions

[mikedn]

If one presumes that the ROS optimization is performed

Indeed, that's an interesting aspect of this issue. There are alternatives to fixed but only under the assumption that the C# compiler performs the optimization.

Well, there's also the possibility to not use lddqu (which AFAIR is kind of pointless on current CPUs) and instead just load the vector by casting the span ref to a ref Vector256<int>. But that's probably a different matter.

[AndyAyersMS]

It might not be that hard to realize we're pinning an RVA static and just remove the pinned bit from the tracking local (since these tracking locals are single def we only need to check when we see the first store to the pinned local). That would let dead stores clean things up.

[mikedn]

since these tracking locals are single def

Hmm, why?

[AndyAyersMS]

Well that's how C# does it, they don't try and reuse these locals. And we can watch for nonstandard uses since we now track single-defness in the importer. ... if local is pinned and store is rva static and local is single def -- unpin.

[mikedn]

Well that's how C# does it, they don't try and reuse these locals

That doesn't seem to be the case:

        fixed (byte* bp = &Mask0.GetPinnableReference())
            mask0 = Avx.LoadDquVector256(bp).AsInt32();
        fixed (byte* bp = &Mask0.GetPinnableReference())
            mask0 = Avx.LoadDquVector256(bp).AsInt32();

generates:

G_M55898_IG01:
       4883EC58             sub      rsp, 88
       C5F877               vzeroupper
       33C0                 xor      rax, rax
       4889442450           mov      qword ptr [rsp+50H], rax
G_M55898_IG02:
       48B8182CA7C6B1010000 mov      rax, 0x1B1C6A72C18
       4889442450           mov      bword ptr [rsp+50H], rax
       C5FFF000             vlddqu   ymm0, ymmword ptr[rax]
       C5FD11442420         vmovupd  ymmword ptr[rsp+20H], ymm0
       33C0                 xor      rax, rax
       4889442450           mov      bword ptr [rsp+50H], rax
       48B8182CA7C6B1010000 mov      rax, 0x1B1C6A72C18
       4889442450           mov      bword ptr [rsp+50H], rax
       C5FFF000             vlddqu   ymm0, ymmword ptr[rax]
       C5FD110424           vmovupd  ymmword ptr[rsp], ymm0
       33C0                 xor      rax, rax
       4889442450           mov      bword ptr [rsp+50H], rax
       33C0                 xor      eax, eax
G_M55898_IG03:
       C5F877               vzeroupper
       4883C458             add      rsp, 88
       C3                   ret

But even without reuse these variables can't be single-def due to the null store at the end of the fixed block, that guarantees that there are always at least 2 defs.

[AndyAyersMS]

Thanks, looks like I was way off here.

From C# there will be only a single reaching def, though saying this is a bit odd since there are no uses in the code (only implicit uses at GC safe points). But it might be hard to show that property holds definitively.

On the other hand, there is no reason to actually store the rva static pointer into the pinning slot, so we could at least get rid of the first stores ... we'd still "unpin" though. Any other pointer that can somehow reach the implicit uses will be stored, and that's sufficient.

[damageboy]

@Suchiman

What you proposed is what I'm actually doing.
I just hide that ungodly mess in a static local function.

I thought it would be useful to report this since I don't know what people's expectation's are w.r.t this.