Convert MemoryMarshal.GetArrayDataReference to a JIT intrinsic

[MichalPetryka]

Converts MemoryMarshal.GetArrayDataReference to
an always expand JIT intrinsic and removes the VM intrinsics.

Introduces JIT tests validating the correct behaviour.

Fixes invalid codegen samples from:
#58312 (comment)

[ghost]

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

Issue Details

---

Converts MemoryMarshal.GetArrayDataReference to
an always expand JIT intrinsic and removes the VM intrinsics.

Introduces JIT tests validating the correct behaviour.

Fixes invalid codegen samples from:
#58312 (comment)

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

[jkotas]

I see the Problem1 and Problem2 tests failing with:

/__w/1/s/src/tests/JIT/Intrinsics/MemoryMarshalGetArrayDataReference.cs:L65 test failed (expected: equal, actual: 2-2).
/__w/1/s/src/tests/JIT/Intrinsics/MemoryMarshalGetArrayDataReference.cs:L82 test failed (expected: equal, actual: 0-0).

both before and after the change. It suggests the problem is somewhere else.

Does the aliasing problem that was mentioned in the original comment actually repro in observable way on current main?

[MichalPetryka]

The tests are missing an ! on Equals there, as you can see the actual values are the same here. I'm going to fix it tomorrow together with the missing NREs and invalid IndexOutOfRanges.

As far as the tests here, @SingleAccretion told me on discord that those should still happen on main, haven't verified it though.

[lambdageek]

@vargaz After we switched the intrinsic to OP_CHECK_THIS in #72897, the LLVM AOT build here is failing

  /__w/1/s/artifacts/bin/mono/Linux.arm64.Release/cross/linux-arm64/opt: mono_aot_a0U4eG/temp.bc: error: Invalid cast (Producer: 'LLVM11.1.0' Reader: 'LLVM 11.1.0')
  AOT of image /__w/1/s/artifacts/tests/coreclr/Linux.arm64.Release/JIT/Intrinsics/MemoryMarshalGetArrayDataReference_r/MemoryMarshalGetArrayDataReference_r.dll failed.

This looks reasonable, doesn't it?

			LLVMBuildLoad2 (builder, IntPtrType (), convert (ctx, lhs, pointer_type (IntPtrType ())), "");

[JulieLeeMSFT]

@MichalPetryka, I marked this as .NET 8. If this should be .NET 7, please let me know.

[MichalPetryka]

.Net 8.0 is the correct target, the PR results in a fair bit of regressions, haven't had time to sit down and figure them out yet.

[ghost]

Draft Pull Request was automatically closed for 30 days of inactivity. Please let us know if you'd like to reopen it.

[tannergooding]

@jkotas or @stephentoub, could you reopen this (its locked and needs a maintainer to do so).

We've got known aliasing bugs around using GetArrayDataReference and so even if this causes some small regressions, it resolves real world bugs caused by us relying on the UB of aliasing reference types.

Ideally we'd take this as is now and then look at improving the codegen and bringing us back to the current numbers before .NET 8 ships.

[tannergooding]

Another example of problematic behavior without this fix is:

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
//

using System;
using System.Numerics;
using System.Runtime.CompilerServices;
using System.Runtime.InteropServices;

internal partial class VectorTest
{
    private static void Main()
    {
        int[] inputArray = CreateArray(17);

        for (int i = 0; i < Vector<int>.Count; i++)
        {
            Console.WriteLine(inputArray[i]);
        }

        Create(out Vector<int> v, inputArray, inputArray.Length);
    }

    public static void Create(out Vector<int> result, int[] values, int index)
    {
        if ((index < 0) || ((values.Length - index) < Vector<int>.Count))
        {
            ThrowArgumentOutOfRangeException();
        }

        result = Unsafe.ReadUnaligned<Vector<int>>(ref Unsafe.As<int, byte>(ref Unsafe.Add(ref MemoryMarshal.GetArrayDataReference(values), index)));
    }

    public static void ThrowArgumentOutOfRangeException()
    {
        throw new ArgumentOutOfRangeException();
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    public static int[] CreateArray(int size) => new int[size];
}

Where the JIT inlines Create(out Vector<int> v, inputArray, inputArray.Length); and believes that inputArray must be null, so it internally asserts and leads to bad codegen in release builds.

[MichalPetryka]

Created a new PR (#79760) since apparently it's not possible to reopen a PR after force pushing.