JIT: LowerHWIntrinsicGetElement can reorder trees unsafely

[jakobbotsch]

The following example throws ArgumentOutOfRangeException when Vector256 is hardware accelerated, but NullReferenceException when it isn't (DOTNET_EnableAVX=0).

public class Program
{
    public static unsafe void Main()
    {
        Foo(null);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static unsafe float Foo(Vector256<float>* v)
    {
        return (*v)[8];
    }
}

Context: #83005 (comment)

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

Issue Details

---

The following example throws ArgumentOutOfRangeException when Vector256 is hardware accelerated, but NullReferenceException when it isn't (DOTNET_EnableAVX=0).

public class Program
{
    public static unsafe void Main()
    {
        Foo(null);
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static unsafe float Foo(Vector256<float>* v)
    {
        return (*v)[8];
    }
}

Context: #83005 (comment)

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

[AndyAyersMS]

The InsertBefore here can reorder the indir and the bounds check, we need to check that the indir is invariant in the range between its old position and new.

runtime/src/coreclr/jit/lowerxarch.cpp

Lines 4462 to 4468 in 6f18b5e

	GenTreeAddrMode* newAddr =
	new (comp, GT_LEA) GenTreeAddrMode(addr->TypeGet(), newBase, newIndex, newScale, newOffset);
	BlockRange().InsertBefore(node, newAddr);
	GenTreeIndir* newIndir =
	comp->gtNewIndir(JITtype2varType(simdBaseJitType), newAddr, (indir->gtFlags & GTF_IND_FLAGS));
	BlockRange().InsertBefore(node, newIndir);

[AndyAyersMS]

Seems like there are potentially two problems here. First we may move op1 (indir) or equvialent below an existing bounds check (this issue). We can fix that by having the indir appear earlier if there is interference.

For the second problem, we may incorporate op2 into the indir and evaluate it before any bounds check. However it appears that any side-effecting computation feeding op2 must be done earlier and op2 will be a temp, since the value needs to be used in both the bounds check and the index. So it should be ok for op2 to be evaluated early.

[AndyAyersMS]

If op2 ends up being very large or negative and is folded into the indir, we might violate the implicit null check rules, so we might fail to properly null check before we bounds check (which presumably will fail for these problematic op2). So perhaps we can't fold op2 into the indir if the indir itself can't be moved down.