JIT: IsContainableHWIntrinsicOp seems to discard many containment opportunities

[jakobbotsch]

See e.g. the following:

runtime/src/coreclr/jit/lowerxarch.cpp

Lines 7921 to 7941 in 5883b72

	case NI_Vector128_CreateScalarUnsafe:
	case NI_Vector256_CreateScalarUnsafe:
	case NI_Vector512_CreateScalarUnsafe:
	{
	if (!varTypeIsIntegral(childNode->TypeGet()))
	{
	// The floating-point overload doesn't require any special semantics
	supportsSIMDScalarLoads = true;
	supportsGeneralLoads = supportsSIMDScalarLoads;
	break;
	}
	// The integral overloads only take GPR/mem
	assert(supportsSIMDScalarLoads == false);
	const unsigned expectedSize = genTypeSize(genActualType(parentNode->GetSimdBaseType()));
	const unsigned operandSize = genTypeSize(childNode->TypeGet());
	supportsGeneralLoads = (operandSize >= expectedSize);
	break;
	}

Using expectedSize = genTypeSize(genActualType(parentNode->GetSimdBaseType())) means we e.g. disallow containing LCL_FLD<ubyte> inside CreateScalar<ubyte> (this particular case is still contained after we transform to broadcast, since broadcast is missing any size check at all -- that's issue #83387). Likely we should not be using actual types here.

The same kind of check is also present for the ConvertScalarToVector128* variants.

[ghost]

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

Issue Details

---

See e.g. the following:

runtime/src/coreclr/jit/lowerxarch.cpp

Lines 7921 to 7941 in 5883b72

	case NI_Vector128_CreateScalarUnsafe:
	case NI_Vector256_CreateScalarUnsafe:
	case NI_Vector512_CreateScalarUnsafe:
	{
	if (!varTypeIsIntegral(childNode->TypeGet()))
	{
	// The floating-point overload doesn't require any special semantics
	supportsSIMDScalarLoads = true;
	supportsGeneralLoads = supportsSIMDScalarLoads;
	break;
	}
	// The integral overloads only take GPR/mem
	assert(supportsSIMDScalarLoads == false);
	const unsigned expectedSize = genTypeSize(genActualType(parentNode->GetSimdBaseType()));
	const unsigned operandSize = genTypeSize(childNode->TypeGet());
	supportsGeneralLoads = (operandSize >= expectedSize);
	break;
	}

Using expectedSize = genTypeSize(genActualType(parentNode->GetSimdBaseType())) means we e.g. disallow containing LCL_FLD<ubyte> inside CreateScalar<ubyte> (this particular case is still contained after we transform to broadcast, since broadcast is missing any size check at all -- that's issue #83387). Likely we should not be using actual types here.

The same kind of check is also present for the ConvertScalarToVector128* variants.

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

[tannergooding]

Some of this is because the underlying instructions don't always take the exact size.

For example, on SSE2 enabled hardware you only have movd and pinsrw as a way of getting an integer value into a SIMD register. movd is exclusively 32/64-bits, while pinsrw is 16-bits.

On SSE4.1 enabled hardware, you get access to pinsrb/pinsrd/pinsrq and that supports 8/32/64-bits.

You don't get broadcast until AVX/AVX2 enabled hardware and that continues supporting all sizes explicitly.

---

For the most part, LowerHWIntrinsicCreateNode (and the other LowerHWIntrinsic* methods) is supposed to take care of taking the various Create nodes and transforming them to the ISA specific cases that can support more explicit checks and scenarios.

There are a few cases where those transforms don't happen and we may need to replicate the necessary ISA checks in IsContainableHWIntrinsicOp for those scenarios, but it really depends on how its being used and what codegen we'll have to generate.

[jakobbotsch]

Some of this is because the underlying instructions don't always take the exact size.

I see -- feel free to close this if there are no additional opportunities to be had here

[tannergooding]

I'll keep it open for now so we can confirm we aren't missing anything obvious

It's definitely possible we are, just wanted to note that some of it should be "by design" here.