JIT: Unnecessary field load for promoted struct

[Zhentar]

_{tl;dr: tried to go plaid, got polka dots} C# code:

  public class TestCase
    {
        public int TestCaseCode()
        {
            int result = 0;
            using (var lineSource = new LineEnumeratorSource())
                foreach (var line in lineSource)
                    result += line.GetString(0).Length;
            return result;
        }

        public class LineEnumeratorSource : IDisposable
        {
            public LineEnumerator GetEnumerator() => new LineEnumerator(ReadOnlySpan<byte>.Empty, 0);
            [MethodImpl(MethodImplOptions.NoInlining)]
            public void Dispose() { }
        }

        public class StringTable
        {
            [MethodImpl(MethodImplOptions.NoInlining)]
            public string GetStringForBytes(ReadOnlySpan<byte> bytes) => "";
        }

        public readonly ref struct SpanLineParser
        {
            private readonly ReadOnlySpan<byte> _span;
            private readonly ReadOnlySpan<int> _colStarts;
            private readonly StringTable _stringTable;

            public SpanLineParser(ReadOnlySpan<byte> memory, ReadOnlySpan<int> colStarts, StringTable strings)
            {
                _span = memory; _colStarts = colStarts; _stringTable = strings;
            }

            [MethodImpl(MethodImplOptions.AggressiveInlining)]
            public string GetString(int col)
            {
                if ((uint)col + 1 < (uint)_colStarts.Length)
                {
                    ref int colStarts = ref MemoryMarshal.GetReference(_colStarts);
                    int from = Unsafe.Add(ref colStarts, col);
                    int end = Unsafe.Add(ref colStarts, col + 1) - 1;

                    return _stringTable.GetStringForBytes(_span.Slice(from, end - from));
                }
                return string.Empty;
            }
        }

        public ref struct LineEnumerator
        {
            private ReadOnlySpan<byte> _actualMemory;
            private readonly StringTable _stringTable;
            private readonly int[] _colStarts;
            private int _delimIndexCount;
            private int _nextIndex;
            public LineEnumerator(ReadOnlySpan<byte> memory, byte delim)
            {
                _actualMemory = memory; _colStarts = new int[128]; _stringTable = new StringTable(); _delimIndexCount = 0; _nextIndex = 1;
            }

            public SpanLineParser Current => GetCurrent();

            [MethodImpl(MethodImplOptions.AggressiveInlining)]
            private SpanLineParser GetCurrent() => new SpanLineParser(_actualMemory.Slice(0, _nextIndex - 1), new ReadOnlySpan<int>(_colStarts, 0, _delimIndexCount), _stringTable);

            [MethodImpl(MethodImplOptions.NoInlining)]
            public bool MoveNext() => false;
        }
    }

Selected ASM:

    xor     eax,eax
    lea     r8,[rbp-0D0h]
    vxorps  xmm0,xmm0,xmm0
    vmovdqu xmmword ptr [r8],xmm0
    vmovdqu xmmword ptr [r8+10h],xmm0
    mov     qword ptr [r8+20h],rax
    mov     rax,qword ptr [rbp-50h]
    lea     r8,[rbp-0C8h]
    mov     qword ptr [r8],rdx
    mov     dword ptr [r8+8],ecx
    lea     rcx,[rbp-0B8h]
    mov     qword ptr [rcx],r14
    mov     dword ptr [rcx+8],ebx
    mov     qword ptr [rbp-0D0h],rax
    vmovdqu xmm0,xmmword ptr [rbp-0D0h]
    vmovdqu xmmword ptr [rbp-78h],xmm0
    vmovdqu xmm0,xmmword ptr [rbp-0C0h]
    vmovdqu xmmword ptr [rbp-68h],xmm0
    mov     rcx,qword ptr [rbp-0B0h]
    mov     qword ptr [rbp-58h],rcx
;     vvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvvv
;     result += line.GetString(0).Length;
    cmp     dword ptr [rbp-58h],1
    jbe     M00_L08
    lea     rcx,[rbp-60h]
    mov     rdx,qword ptr [rcx]
    mov     ecx,dword ptr [rcx+8]
    mov     ecx,dword ptr [rdx]
    mov     edx,dword ptr [rdx+4]
    dec     edx

I see a few issues here...

1. Zeroing out the struct immediately before writing to all of the fields (pretty sure I've seen an issue for this already)
2. Copying the struct before the GetString call. It's a readonly struct so it shouldn't need a defensive copy, but even if it did, it's not referenced after the call so it wouldn't be necessary, and even if it were, the call is inlined so the lack of modification should be detectable.
3. I'm no x86 assembly expert but I'm pretty sure that mov ecx,dword ptr [rcx+8] is completely pointless since it's immediately overwritten.

[voinokin]

@Zhentar Have you tried to check IL ?

[Zhentar]

I have, but I don't know what I'm looking for so it didn't help much. The struct copy looks like this:

          IL_0023: stloc.s      line
          IL_0025: ldloc.0      // result
          IL_0026: ldloca.s     line

[Zhentar]

Issue 1 is dotnet/runtime#8186 and 2 is dotnet/runtime#4308

For the third, in ref int colStarts = ref MemoryMarshal.GetReference(_colStarts);, the span _colStarts is being promoted; mov ecx,dword ptr [rcx+8] is loading the _length field of span even though it's not ever used. My searches haven't turned up any issues for that, but I could just be using the wrong terms.

[AndyAyersMS]

IIRC when we promote a struct we load all its fields like this, and yeah, it seems unnecessary. We should investigate.

[AndyAyersMS]

The unnecessary load is not there anymore.

; Assembly listing for method TestCase:TestCaseCode():int:this

G_M15559_IG15:
       cmp      dword ptr [rbp-50H], 1
       jbe      G_M15559_IG24
       lea      rcx, bword ptr [rbp-58H]
       mov      rcx, bword ptr [rcx]
       mov      edx, dword ptr [rcx]
       mov      ecx, dword ptr [rcx+4]           
       dec      ecx

Not sure when the codegen changed, perhaps from better quality ref counts...(#19345) ??

Think we can close this as the other CQ issues here are tracked.

@Zhentar feel free to take a fresh look.

[AndyAyersMS]

@Zhentar I'm going to go ahead and close this one. Feel free to re-open when/if you've had a chance to take a look.

[Zhentar]

It also looks to me like it has been resolved 👍