JIT: Inlining may produce incorrectly byref-typed SUB trees

[jakobbotsch]

See #82166 (comment):

STMT00051 ( 0x000[E-] ... ??? ) <- INL02 @ 0x000[E-] <- INLRT @ 0x0A1[E-]
               [000177] -A---------                         ▌  ASG       long  
               [000176] D------N---                         ├──▌  LCL_VAR   long   V10 tmp1         
               [000175] -----------                         └──▌  SUB       byref 
               [000173] -----------                            ├──▌  FIELD_ADDR byref  SequentialLayoutMinPacking`1[ushort]:_byte
               [000174] -----------                            │  └──▌  LCL_VAR_ADDR byref  V04 loc1         
               [000172] -----------                            └──▌  LCL_VAR_ADDR long   V04 loc1         

impGetByRefResultType bases its result on the type of operands, yet the inliner can directly substitute argument trees in some cases causing this to not really work out in the expected way.

[ghost]

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

Issue Details

---

See #82166 (comment):

STMT00051 ( 0x000[E-] ... ??? ) <- INL02 @ 0x000[E-] <- INLRT @ 0x0A1[E-]
               [000177] -A---------                         ▌  ASG       long  
               [000176] D------N---                         ├──▌  LCL_VAR   long   V10 tmp1         
               [000175] -----------                         └──▌  SUB       byref 
               [000173] -----------                            ├──▌  FIELD_ADDR byref  SequentialLayoutMinPacking`1[ushort]:_byte
               [000174] -----------                            │  └──▌  LCL_VAR_ADDR byref  V04 loc1         
               [000172] -----------                            └──▌  LCL_VAR_ADDR long   V04 loc1         

impGetByRefResultType bases its result on the type of operands, yet the inliner can directly substitute argument trees in some cases causing this to not really work out in the expected way.

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

[jakobbotsch]

This might not be too hard to fix; the actual signature types are available in the StackEntry that is popped right above the call to impGetByRefResultType. They are currently ignored, but really should be used to determine the target type instead of basing it on the type of the trees, which doesn't take implicit TYP_BYREF<->TYP_I_IMPL casts into account.

[EgorBo]

Moving to 9.0 (will see if I have time to land this in 8.0 as the best effort)

[EgorBo]

@jakobbotsch I can't reproduce it on Main it seems, SUB gets typed as long for me:

               [000003] DA---------                         *  STORE_LCL_VAR long   V02 loc0         
               [000002] -----------                         \--*  SUB       long  
               [000001] -----------                            +--*  LCL_VAR   byref  V01 arg1          (last use)
               [000000] -----------                            \--*  LCL_VAR   byref  V00 arg0          (last use)

[EgorBo]

I was playing with this repro:

using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;
using System.Runtime.InteropServices;

public class Program
{
    public static void Main()
    {
        new SequentialLayoutMinPacking<ushort>().OffsetOfByte();
        new SequentialLayoutMinPacking<ushort>().OffsetOfValue();
    }

    internal static int OffsetOf<T, U>(ref T origin, ref U target)
    {
        return Unsafe.ByteOffset(ref origin, ref Unsafe.As<U, T>(ref target)).ToInt32();
    }
}


[StructLayout(LayoutKind.Sequential, Pack = 1)]
struct SequentialLayoutMinPacking<T>
{
    public byte _byte;
    public T _value;

    public int Size => Unsafe.SizeOf<SequentialLayoutMinPacking<T>>();

    [MethodImpl(MethodImplOptions.NoInlining)]
    public int OffsetOfByte() => Program.OffsetOf(ref this, ref _byte);

    [MethodImpl(MethodImplOptions.NoInlining)]
    public int OffsetOfValue() => Program.OffsetOf(ref this, ref _value);
}

[EgorBo]

Spoke with @jakobbotsch offline, it seems that #89324 has fixed it and it's no longer possible to reproduce it in C#/Unsafe, only with raw IL. So moving to future for now.