VN does not number some HWI loads properly

[SingleAccretion]

Reproduction:

using System;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;

unsafe
{
    var dbls = stackalloc double[] { 1, 1 };
    Console.WriteLine(Problem(dbls));
}

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static unsafe bool Problem(double* a)
{
    const double Mask = -0.0;

    var v1 = Avx.MaskLoad(a, Vector128.Create(0, Mask));
    if (v1.GetElement(0) == 0)
    {
        var v2 = Avx.MaskLoad(a, Vector128.Create(Mask, 0));
        if (v2.GetElement(0) == 0)
        {
            return true;
        }
    }

    return false;
}

Expected result: false.
Actual result: true.

Cause: in the below VN code we only encode the value of the address operand:

runtime/src/coreclr/jit/valuenum.cpp

Lines 9601 to 9622 in a71f261

	if (isMemoryLoad)
	{
	ValueNumPair op1vnp;
	ValueNumPair op1Xvnp;
	vnStore->VNPUnpackExc(tree->Op(1)->gtVNPair, &op1vnp, &op1Xvnp);
	// The addrVN incorporates both op1's ValueNumber and the func operation
	// The func is used because operations such as LoadLow and LoadHigh perform
	// different operations, thus need to compute different ValueNumbers
	// We don't need to encode the result type as it will be encoded by the opcode in 'func'
	//
	ValueNum addrVN = vnStore->VNForFunc(TYP_BYREF, func, op1vnp.GetLiberal());
	// The address could point anywhere, so it is an ByrefExposed load.
	//
	ValueNum loadVN = fgValueNumberByrefExposedLoad(tree->TypeGet(), addrVN);
	tree->gtVNPair.SetLiberal(loadVN);
	tree->gtVNPair.SetConservative(vnStore->VNForExpr(compCurBB, tree->TypeGet()));
	tree->gtVNPair = vnStore->VNPWithExc(tree->gtVNPair, op1Xvnp);
	fgValueNumberAddExceptionSetForIndirection(tree, tree->Op(1));
	return;
	}

We need to encode the value of the second parameter (if it is present) as well.

FYI @tannergooding

[ghost]

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

Issue Details

---

Reproduction:

using System;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;

unsafe
{
    var dbls = stackalloc double[] { 1, 1 };
    Console.WriteLine(Problem(dbls));
}

[MethodImpl(MethodImplOptions.NoInlining | MethodImplOptions.AggressiveOptimization)]
static unsafe bool Problem(double* a)
{
    const double Mask = -0.0;

    var v1 = Avx.MaskLoad(a, Vector128.Create(0, Mask));
    if (v1.GetElement(0) == 0)
    {
        var v2 = Avx.MaskLoad(a, Vector128.Create(Mask, 0));
        if (v2.GetElement(0) == 0)
        {
            return true;
        }
    }

    return false;
}

Expected result: false.
Actual result: true.

Cause: in the below VN code we only encode the value of the address operand:

runtime/src/coreclr/jit/valuenum.cpp

Lines 9601 to 9622 in a71f261

	if (isMemoryLoad)
	{
	ValueNumPair op1vnp;
	ValueNumPair op1Xvnp;
	vnStore->VNPUnpackExc(tree->Op(1)->gtVNPair, &op1vnp, &op1Xvnp);
	// The addrVN incorporates both op1's ValueNumber and the func operation
	// The func is used because operations such as LoadLow and LoadHigh perform
	// different operations, thus need to compute different ValueNumbers
	// We don't need to encode the result type as it will be encoded by the opcode in 'func'
	//
	ValueNum addrVN = vnStore->VNForFunc(TYP_BYREF, func, op1vnp.GetLiberal());
	// The address could point anywhere, so it is an ByrefExposed load.
	//
	ValueNum loadVN = fgValueNumberByrefExposedLoad(tree->TypeGet(), addrVN);
	tree->gtVNPair.SetLiberal(loadVN);
	tree->gtVNPair.SetConservative(vnStore->VNForExpr(compCurBB, tree->TypeGet()));
	tree->gtVNPair = vnStore->VNPWithExc(tree->gtVNPair, op1Xvnp);
	fgValueNumberAddExceptionSetForIndirection(tree, tree->Op(1));
	return;
	}

We need to encode the value of the second parameter (if it is present) as well.

FYI @tannergooding

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

[SingleAccretion]

VN also doesn't account for the fact LoadHigh-family of intrinsics have the address as the second operand, not the first.

[SingleAccretion]

Another issue: "helper" intrinsics, i. e. the recently added V128 ones, aren't accounted as a memory load for at all.

[tannergooding]

Another issue: "helper" intrinsics, i. e. the recently added V128 ones, aren't accounted as a memory load for at all.

These ones should be handled on import by converting them to the relevant actual intrinsics.