JIT: emitter assert running PMI

[AndyAyersMS]

Compile attached test case (from #7279, requires /langversion:latest) and jit via PMI to get the following emitter assert:

D:\repos\coreclr2\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root\corerun.exe d:\repos\jitutils\bin\PMI.dll prepall-quiet d:\bugs\9066\ex3.exe

Assert failure(PID 19908 [0x00004dc4], Thread: 22384 [0x5770]): Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' in 'SpanLike`1[Vector`1][System.Numerics.Vector`1[System.Single]]:get_Item(int):byref:this' (IL size 74)

    File: d:\repos\coreclr2\src\jit\emitxarch.cpp Line: 11498
    Image: D:\repos\coreclr2\bin\tests\Windows_NT.x64.Checked\Tests\Core_Root\CoreRun.exe

This assert is new in the past 10 months or so... (edit: probably not relevant -- we didn't have PMI back then, and this instantiation is not created when you run the test normally).

cc @dotnet/jit-contrib

Test case

using System;
using System.Diagnostics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;
using System.Text;

class Program
{
    static void Main(string[] args)
    {
        var bytes = Encoding.ASCII.GetBytes("Hello World! Testing stack space!");
        Console.WriteLine(OneSpantype(bytes));
        Console.WriteLine(LotsOfSpans(bytes));
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static byte OneSpantype(byte[] array)
    {
        var span = new SpanLike<byte>(array);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        span = span.Slice(1);
        return span[0];
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static byte LotsOfSpans(byte[] array)
    {
        var span00 = new SpanLike<byte>(array);
        var span01 = span00.Slice(1);
        var span02 = span01.Slice(1);
        var span03 = span02.Slice(1);
        var span04 = span03.Slice(1);
        var span05 = span04.Slice(1);
        var span06 = span05.Slice(1);
        var span07 = span06.Slice(1);
        var span08 = span07.Slice(1);
        var span09 = span08.Slice(1);
        var span10 = span09.Slice(1);
        var span11 = span10.Slice(1);
        var span12 = span11.Slice(1);
        var span13 = span12.Slice(1);
        var span14 = span13.Slice(1);
        var span15 = span14.Slice(1);
        var span16 = span15.Slice(1);
        var span17 = span16.Slice(1);
        var span18 = span17.Slice(1);
        var span19 = span18.Slice(1);
        var span20 = span19.Slice(1);
        var span21 = span20.Slice(1);
        var span22 = span21.Slice(1);
        var span23 = span22.Slice(1);
        var span24 = span23.Slice(1);
        var span25 = span24.Slice(1);
        var span26 = span25.Slice(1);
        var span27 = span26.Slice(1);
        var span28 = span27.Slice(1);
        var span29 = span28.Slice(1);
        var span30 = span29.Slice(1);
        return span30[0];
    }
}

public readonly ref struct SpanLike<T>
{
    private readonly Pinnable<T> _pinnable;
    private readonly IntPtr _byteOffset;
    private readonly int _length;

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public SpanLike(T[] array)
    {
        if (array == null)
            ThrowHelper.ThrowArgumentNullException(ExceptionArgument.array);
        if (default(T) == null && array.GetType() != typeof(T[]))
            ThrowHelper.ThrowArrayTypeMismatchException_ArrayTypeMustBeExactMatch(typeof(T));

        _length = array.Length;
        _pinnable = Unsafe.As<Pinnable<T>>(array);
        _byteOffset = SpanHelpers.PerTypeValues<T>.ArrayAdjustment;
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    internal SpanLike(Pinnable<T> pinnable, IntPtr byteOffset, int length)
    {
        Debug.Assert(length >= 0);

        _length = length;
        _pinnable = pinnable;
        _byteOffset = byteOffset;
    }

    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public SpanLike<T> Slice(int start)
    {
        if ((uint)start > (uint)_length)
            ThrowHelper.ThrowArgumentOutOfRangeException(ExceptionArgument.start);

        IntPtr newOffset = _byteOffset.Add<T>(start);
        int length = _length - start;
        return new SpanLike<T>(_pinnable, newOffset, length);
    }

    public ref T this[int index]
    {
        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        get
        {
            if ((uint)index >= ((uint)_length))
                ThrowHelper.ThrowIndexOutOfRangeException();

            if (_pinnable == null)
                unsafe { return ref Unsafe.Add<T>(ref Unsafe.AsRef<T>(_byteOffset.ToPointer()), index); }
            else
                return ref Unsafe.Add<T>(ref Unsafe.AddByteOffset<T>(ref _pinnable.Data, _byteOffset), index);
        }
    }
}

[StructLayout(LayoutKind.Sequential)]
internal sealed class Pinnable<T>
{
    public T Data;
}

internal static partial class SpanHelpers
{
    [MethodImpl(MethodImplOptions.AggressiveInlining)]
    public static IntPtr Add<T>(this IntPtr start, int index)
    {
        Debug.Assert(start.ToInt64() >= 0);
        Debug.Assert(index >= 0);

        unsafe
        {
            if (sizeof(IntPtr) == sizeof(int))
            {
                // 32-bit path.
                uint byteLength = (uint)index * (uint)Unsafe.SizeOf<T>();
                return (IntPtr)(((byte*)start) + byteLength);
            }
            else
            {
                // 64-bit path.
                ulong byteLength = (ulong)index * (ulong)Unsafe.SizeOf<T>();
                return (IntPtr)(((byte*)start) + byteLength);
            }
        }
    }

    public class PerTypeValues<T>
    {
        public static readonly IntPtr ArrayAdjustment = MeasureArrayAdjustment();

        // Array header sizes are a runtime implementation detail and aren't the same across all runtimes. (The CLR made a tweak after 4.5, and Mono has an extra Bounds pointer.)
        private static IntPtr MeasureArrayAdjustment()
        {
            T[] sampleArray = new T[1];
            return Unsafe.ByteOffset<T>(ref Unsafe.As<Pinnable<T>>(sampleArray).Data, ref sampleArray[0]);
        }
    }
}

internal static class ThrowHelper
{
    internal static void ThrowArgumentNullException(ExceptionArgument argument) { throw CreateArgumentNullException(argument); }
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static Exception CreateArgumentNullException(ExceptionArgument argument) { return new ArgumentNullException(argument.ToString()); }

    internal static void ThrowArrayTypeMismatchException_ArrayTypeMustBeExactMatch(Type type) { throw CreateArrayTypeMismatchException_ArrayTypeMustBeExactMatch(type); }
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static Exception CreateArrayTypeMismatchException_ArrayTypeMustBeExactMatch(Type type) { return new ArrayTypeMismatchException(); }

    internal static void ThrowIndexOutOfRangeException() { throw CreateIndexOutOfRangeException(); }
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static Exception CreateIndexOutOfRangeException() { return new IndexOutOfRangeException(); }

    internal static void ThrowArgumentOutOfRangeException(ExceptionArgument argument) { throw CreateArgumentOutOfRangeException(argument); }
    [MethodImpl(MethodImplOptions.NoInlining)]
    private static Exception CreateArgumentOutOfRangeException(ExceptionArgument argument) { return new ArgumentOutOfRangeException(argument.ToString()); }
}

internal enum ExceptionArgument
{
    array,
    start
}

category:correctness
theme:ir
skill-level:expert
cost:large

[AndyAyersMS]

Had forgotten about this one and hoped some of the recent fixes in this area (eg dotnet/coreclr#21217 or dotnet/coreclr#21477) might have fixed it, but it still repros.

In SpanLike`1[Vector`1][System.Numerics.Vector`1[System.Single]]:get_Item(int):byref:this

we see tmp2 defined as a long and then used as a byref:

[000067] ------------              *  STMT      void  (IL   ???...  ???)
[000058] ---XG-------              |  /--*  FIELD     long   _value
[000057] ------------              |  |  \--*  LCL_VAR   byref  V03 tmp1         
[000066] -A-XG-------              \--*  ASG       long  
[000065] D------N----                 \--*  LCL_VAR   long   V04 tmp2         

[000052] ------------              *  STMT      void  (IL   ???...  ???)
[000051] --C---------              \--*  RETURN    byref 
[000072] ------------                 |     /--*  CAST      long <- int
[000071] ------------                 |     |  \--*  CNS_INT   int    32
[000074] ------------                 |  /--*  MUL       long  
[000073] ------------                 |  |  \--*  CAST      long <- int
[000045] ------------                 |  |     \--*  LCL_VAR   int    V01 arg1         
[000075] ------------                 \--*  ADD       byref 
[000070] ------------                    \--*  LCL_VAR   byref  V04 tmp2         

Here tmp1 is a byref to an IntPtr and so tmp2 is the void* typed _value field.

tmp2 comes from here:

Importing BB08 (PC=000) of 'System.Runtime.CompilerServices.Unsafe:AsRef(long):byref'
    [ 0]   0 (0x000) ldarg.0
lvaGrabTemp returning 4 (V04 tmp2) called for Inlining Arg.
    [ 1]   1 (0x001) ret

    Inlinee Return expression (before normalization)  =>
               [000064] ------------              *  LCL_VAR   long   V04 tmp2         
Allowing return type mismatch: have long, needed byref

Unsafe.AsRef creates the potential for missing the birth of a byref as it's just a retyping operator and we will often forward the arg as return value to the downstream consumers and ignore that return type mismatch (as noted in the jit dump).

So perhaps we need some kind of special cast here in jit IR?

[mikedn]

Unsafe.AsRef creates the potential for missing the birth of a byref

But does that actually matter? The value must have been a native pointer to begin with and changing its type to byref shouldn't have any effect (e.g. not reporting it to the GC won't make a difference because it isn't a managed pointer).

[AndyAyersMS]

Agree there's probably not a correctness issue here -- it's more a question of how robust the jit can be about tracking an asserting GC consistency.

[AndyAyersMS]

Will take another look at this.

[AndyAyersMS]

I think the rough plan for this kind of issue is to have a phase that tries to provide a globally consistent view of byrefs and native ints -- perhaps generalizing the kind of type updating that @erozenfeld introduced as part of the stack allocation work.

The importer/inliner has too limited of a window on things to fix this properly. Once we tolerate a mismatch (say at an inline) we can't easily tell what all needs to change to restore order.

This rewriting is probably too ambitious to fit into 3.0.

Opened dotnet/coreclr#22837 (#12121) to consider implementing a type checker, dotnet/coreclr#22838 (aka #12122) to implement a type rewriter.

[BruceForstall]

22837 => #12121

[AndyAyersMS]

Still repros:

set complus_tieredcompilation=0
corerun.exe pmi.dll prepall-quiet 10821.exe

Assert failure(PID 20368 [0x00004f90], Thread: 16840 [0x41c8]): 
   Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' 
   in 'SpanLike`1[Byte][System.Byte]:get_Item(int):byref:this' 
   during 'Emit code' (IL size 74)

    File: C:\repos\runtime0\src\coreclr\src\jit\emitxarch.cpp Line: 11453

[AndyAyersMS]

Seems many of these cases are introduced by arg passing during inlining. Looking at handling them by modifying impInlineFetchArg to not retype but to use a new temp.

[AndyAyersMS]

That handles one class of changes, but we can also see this retyping for methods like Unsafe.AsRef that retype via local reinterpretation:

IL_0000  02                ldarg.0        // long
IL_0001  0a                stloc.0        // byref
IL_0002  06                ldloc.0     
IL_0003  2a                ret         

We can see this assert fire if we have a long chain of temp copies, reinterpret from long to byref along the way, and then add to the byref, eg

V08 (long) <- V63
V11 (byref) <- V08 (long)   // retyped by importer, see last bit of logic under _PopValue:
V09 (byref) <- V11 (byref)
V07 (byref) <- V09 (byref) + V03 (int)

Say we allocate all the RHS temps into RDX. During codegen we see the switch from long to byref, but don't emit any code for it. The last instruction emits an add which is marked as a GC instruction.

But the emitter GC tracking only tracks what we actually emit, which is

RDX (long) <- ...
RDX (byref) <- RDX(long) + RAX   ==> assert, RDX is not a byref

A couple possible ideas.

1. We could try and rewrite long/byref appearances starting from some "source of truth" (likely INDIRS/CALLS/ARGS) so that all the types are consistent; if there are multiple reaching def types then the type needs to end up as BYREF to be fully safe. We probably can't afford to do this rewriting in debug/tier0 but since it seems these inconsistencies mainly arise from inlining perhaps we don't need to run the rewriting it unless we optimize. (this is described via JIT: consider phase to clean up mixed byref/native int uses #12122 -- we might be able to leverage the rewriting we'd do for object stack allocation here).

2. We could add a "no encoding" mov that just captures RDX becoming a BYREF so the emitter can model it.

3. Or, for a lower tech approach, that weakens the assert but doesn't get rid of it entirely -- when we inline and see one of these type mismatch cases (in particular going from long->byref) we can flag the method in some way, and just suppress the assert.

And some thoughts on testing:

Likely most cases of this are benign because the values are stack references and don't need tracking. But we might see actual GC holes for methods with implicit byref args that are invoked by reflection.

[BruceForstall]

Having done much less analysis and understanding on this issue than you... it seems like option #2 above is preferable as it is (a) simple, and (b) doesn't lose important type information that seems useful to maintain for modeling. It seems like we should always maintain all type conversions explicitly in the IR (even the late "IR"/emitter) -- I think we don't fully follow this "rule" even in the IR today, where type casts are in some cases omitted (?).

[AndyAyersMS]

Here's a bit of the codegen trace from the SPMI example you pointed me at:

IN000f:        mov      rdx, qword ptr [V67 rsp+28H]
                                                              /--*  t174   long   
Generating: N059 (  3,  3) [000176] DA--G-------              *  STORE_LCL_VAR long   V08 tmp4         d:2 rdx REG rdx
							V08 in reg rdx is becoming live  [000176]
							Live regs: 000000C1 {rax rsi rdi} => 000000C5 {rax rdx rsi rdi}
							Live vars: {V00 V01 V03} => {V00 V01 V03 V08}
genIPmappingAdd: ignoring duplicate IL offset 0x13
Generating: N061 (???,???) [000901] ------------                 IL_OFFSET void   IL offset: 0x13 REG NA
Generating: N063 (  1,  1) [000198] ------------       t198 =    LCL_VAR   byref  V08 tmp4         u:2 rdx (last use) REG rdx $480
                                                              /--*  t198   byref  
Generating: N065 (  1,  3) [000200] DA----------              *  STORE_LCL_VAR byref  V11 tmp7         d:2 rdx REG rdx
							V08 in reg rdx is becoming dead  [000198]
							Live regs: 000000C5 {rax rdx rsi rdi} => 000000C1 {rax rsi rdi}
							Live vars: {V00 V01 V03 V08} => {V00 V01 V03}
							V11 in reg rdx is becoming live  [000200]
							Live regs: 000000C1 {rax rsi rdi} => 000000C5 {rax rdx rsi rdi}
							Live vars: {V00 V01 V03} => {V00 V01 V03 V11}
							Byref regs: 00000080 {rdi} => 00000084 {rdx rdi}

Codegen would emit this no-op mov for node [000200] where RDX goes live (nogc) /dead / live (gc). There'd be a new clause in genCodeForStoreLclVar that would check for this case (registers match but GCness of source is not reflected in the codegen register tracking).

However, I'm not sure this really adds anything useful, other than more surgically quieting the emitter assert. We still might be losing track of byrefs in the code upstream of this point.

[sandreenko]

Codegen would emit this no-op mov for node [000200] where RDX goes live (nogc) /dead / live (gc). There'd be a new clause in genCodeForStoreLclVar that would check for this case (registers match but GCness of source is not reflected in the codegen register tracking).

If we can add "no encoding" move in an isolated PR we would be able to then remove cast GC away temps, it will give us some positive diffs. Then we can add a flag and remove retyping (#48675) without regressions (or with a small amount of them).

[AndyAyersMS]

Recent repro:

c:\repos\runtime1\artifacts\tests\coreclr\windows.x64.Release\tests\Core_Root\crossgen2\crossgen2.exe --out foo.dll --jitpath c:\repos\runtime1\artifacts\tests\coreclr\windows.x64.Checked\tests\Core_Root\clrjit.dll -- c:\repos\runtime1\artifacts\tests\coreclr\windows.x64.Release\tests\Core_Root\System.Private.CoreLib.dll
C:\repos\runtime1\src\coreclr\jit\emitxarch.cpp:11739
Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' in 'System.Text.Encoding:GetCharCountWithFallback(long,int,int):int:this' during 'Emit code' (IL size 25)

C:\repos\runtime1\src\coreclr\jit\emitxarch.cpp:11739
Assertion failed '((regMask & emitThisGCrefRegs) && (ins == INS_add)) || ((regMask & emitThisByrefRegs) && (ins == INS_add || ins == INS_sub))' in 'System.Text.Encoding:GetCharsWithFallback(long,int,long,int,int,int,System.Text.DecoderNLS):int:this' during 'Emit code' (IL size 183)

[AndyAyersMS]

I think in the case above this is a spurious assert. We have a last-use byref in RDX and a long in R15 that will become a byref, and we kill the use before we make the dest live, so there's a window where neither is live...

                                                              /--*  t203   long   
                                                              +--*  t226   byref  
Generating: N065 (  4,  5) [000227] ------------       t227 = *  ADD       byref  REG r15 $280
							V68 in reg rdx is becoming dead  [000226]
							Live regs: 000043EC {rdx rbx rbp rsi rdi r8 r9 r14} => 000043E8 {rbx rbp rsi rdi r8 r9 r14}
							Live vars: {V00 V02 V03 V04 V06 V07 V26 V68} => {V00 V02 V03 V04 V06 V07 V26}
							Byref regs: 00000004 {rdx} => 00000000 {}
IN0009:        add      r15, rdx
							Byref regs: 00000000 {} => 00008000 {r15}

and this is where we assert later on.

In practice RDX will stay GC live after this instruction since we have a "lazy kill" model.

[tannergooding]

Reopening since the PR had to be reverted