Decouple `FEATURE_HW_INTRINSICS` from `FEATURE_SIMD`

[tannergooding]

There are a few places in the code-base where FEATURE_HW_INTRINSICS functionality is dependent on FEATURE_SIMD also being defined.

The hardware intrinsics feature should (ideally) share code with the SIMD feature, where possible, but should not be dependent on it being defined.

category:implementation
theme:vector-codegen
skill-level:intermediate
cost:small

[tannergooding]

FYI. @fiigii

[fiigii]

We are reusing TYP_SIMD32 and TYP_SIMD16, so it is impossible to decouple Vector128/256<T> intrinsics from FEATURE_SIMD. At this time, FEATURE_HW_INTRINSICS platforms are always the subset of FEATURE_SIMD platforms, so this coupling looks okay to me.

[tannergooding]

We are reusing TYP_SIMD32 and TYP_SIMD16, so it is impossible to

All of the FEATURE_SIMD stuff (including TYP_SIMD32 and TYP_SIMD16) is behind #ifdef FEATURE_SIMD, so it should just be finding the places that FEATURE_HW_INTRINSICS also depends on and changing it to #if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS), correct?

At this time, FEATURE_HW_INTRINSICS platforms are always the subset of FEATURE_SIMD platforms, so this coupling looks okay to me.

If that is expected to be the case long term, we should likely have some asserts/etc to ensure that FEATURE_SIMD is turned on when FEATURE_HW_INTRINSICS is. Currently, it is possible to turn off FEATURE_SIMD, but not turn off FEATURE_HW_INTRINSICS and the build will break. (However, I am not convinced that coupling them should be the long term case).

[fiigii]

it should just be finding the places that FEATURE_HW_INTRINSICS also depends on and changing it to #if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS), correct?

Yes, and I tried. I remember that is not such trivial...
So the purpose is to build with "turn off FEATURE_SIMD, but not turn off FEATURE_HW_INTRINSICS"? That means only enable scalar HW intrinsic?

[tannergooding]

So the purpose is to build with "turn off FEATURE_SIMD, but not turn off FEATURE_HW_INTRINSICS"? That means only enable scalar HW intrinsic?

No.

FEATURE_SIMD (and COMPlus_FeatureSIMD), as best as I can tell, is primarily used to support the vector types in System.Numerics.Vector. The https://github.com/dotnet/coreclr/blob/master/Documentation/project-docs/clr-configuration-knobs.md documents it as:

Enable SIMD support with companion SIMDVector.dll

My proposal is to decouple them completely such that one or the other can be enabled or disabled without impacting the other.

Realistically, the hardware intrinsics are providing a way to write type-safe inline psuedo-assembly. In order to support that we are essentially adding specialized codegen and compiler support for multiple hardware instructions. Additionally, each hardware intrinsic generally represents a single underlying hardware instruction, so this provides an easy way to declare chains of instructions that will undergo the various compiler stages (rather than requiring an intermediate node that is processed and finally emitted). Because of this, my opinion is that the hardware intrinsics are a feature that other features or code should be dependent on (and not the other way around).

[fiigii]

COMPlus_FeatureSIMD is a different story, and I already addressed it in IsSupported and exceptions.

[fiigii]

@CarolEidt any comment?

[4creators]

the hardware intrinsics are a feature that other features or code should be dependent on (and not the other way around)

My understanding is that the final goal is to implement Vector with underlying support of Hardware Intrinsics, therefore, FEATURE_SIMD should depend on FEATURE_HW_INTRINSICS unless we would merge them into single feature. The expectation is that System.Numerics.Vectors functionality will expand soon to provide support for wide, cross platform functionality based on Intel and Arm hardware intrinsics.

[CarolEidt]

My understanding is that the final goal is to implement Vector with underlying support of Hardware Intrinsics

I don't think it is clear that that would be an improvement, and is certainly not a high priority to make that switch.

That said, I don't think it's desirable to have FEATURE_HW_INTRINSICS depend on FEATURE_SIMD.

it should just be finding the places that FEATURE_HW_INTRINSICS also depends on and changing it to #if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS), correct?

Yes, I think that's the right approach, such that FEATURE_HW_INTRINSICS can be used with FEATURE_SIMD disabled. I'm not sure that disabling FEATURE_SIMD is even a necessary scenario at this point, but there's no reason we should get rid of the ability to build without it.

So the purpose is to build with "turn off FEATURE_SIMD, but not turn off FEATURE_HW_INTRINSICS"? That means only enable scalar HW intrinsic?

I don't think this would imply enabling only scalar HW intrinsics. I would think that FEATURE_SIMD would control intrinsic recognition of just the System.Numerics.Vectors types.

Another possibility would be to define an addition feature, e.g. FEATURE_VECTOR_REGS or something like that, but I don't see why the #if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS) approach shouldn't work, though I admit that I haven't attempted it.

"Enable SIMD support with companion SIMDVector.dll"

Wow - that's an old comment (also here: https://github.com/dotnet/coreclr/blob/master/src/inc/clrconfigvalues.h#L542) that dates back to my original prototype of the feature! It should be updated to System.Numerics.Vectors.dll.

[CarolEidt]

Submitted dotnet/coreclr#15699 to fix the description of the feature.

[tannergooding]

I don't think it is clear that that would be an improvement, and is certainly not a high priority to make that switch.

I agree.

I, personally, think that there are some benefits to doing so (at the very least in the maintainability area). However, that requires prototyping (I have this locally, but it requires HWIntrinsics to be more generally implemented first), testing, etc.

Another possibility would be to define an addition feature, e.g. FEATURE_VECTOR_REGS or something like that, but I don't see why the #if defined(FEATURE_SIMD) || defined(FEATURE_HW_INTRINSICS) approach shouldn't work, though I admit that I haven't attempted it.

I think, long term, it might be beneficial to have them (the core/shared bits, such as TYP_SIMD16 and TYP_SIMD32) behind their own flag (which is set centrally based on other flags being set) or not behind a flag at all. The core/shared bits here are useful in multiple scenarios such as SIMD, HWIntrinsics, AutoVectorization, etc. So decentralizing them from any one feature probably makes sense.

[tannergooding]

Moved to the HWIntrinsics project.

[sdmaclea]

FEATURE_SIMD should be split into FEATURE_SIMD and FEATURE_VECTOR_T_SIMD.

[tannergooding]

I was thinking FEATURE_SIMD_REGISTER (which we might just want outside a feature flag at this point) and FEATURE_SIMD_INTRINSIC (which we might also be able to get rid of, if we eventually switch these to be powered by HWIntrinsics instead).

In either case, its probably worth a more in depth discussion before someone goes and does this work.

[CarolEidt]

On further consideration, I think it would be best to have:

• FEATURE_SIMD imply (only) support for vector register types.
• FEATURE_SIMD_HW (or something) imply support for HW intrinsics that utilize vector register types.
• FEATURE_SIMD_ABSTRACT (or ...) imply support for the target-independent vector types.

We could easily have static asserts that we don't have the 2nd or 3rd without the first.