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.

[tannergooding]

@CarolEidt, so:

• FEATURE_SIMD would cover TYP_SIMD* and supporting code
• FEATURE_SIMD_HW would cover System.Runtime.Intrinsics
  • Would this only be SIMD hardware intrinsics or all hardware intrinsics (things like CRC32 fall into the latter category, but not the first)
• FEATURE_SIMD_ABSTRACT would cover System.Numerics.Vector*

Is that about right?

[CarolEidt]

@tannergooding - yes, that's what I was thinking.
I would think that FEATURE_SIMD_HW would cover only SIMD hardware intrinsics. I think we still need FEATURE_HW_INTRINSICS. So, FEATURE_SIMD_HW would depend on both FEATURE_SIMD and FEATURE_HW_INTRINSICS.

It makes for more FEATURE tags, but I think it would be clearer what each implies.

[tannergooding]

It makes for more FEATURE tags, but I think it would be clearer what each implies.

I think the premise is generally good, but I worry about FEATURE_SIMD_HW and FEATURE_HW_INTRINSICS, as they are basically a circular dependency. That is, due to the way the IsSupported flags are setup and ISAs are exposed, we couldn't have one without the other for most cases (at least for x86/x64).

[sdmaclea]

we couldn't have one without the other for most cases

Not clear why.

I think the premise is generally good

I think @CarolEidt proposed names are unclear. How about?

FEATURE_SIMD_ABSTRACT -> FEATURE_SIMD_NUMERICS
FEATURE_SIMD_HW -> FEATURE_SIMD_HW_INTRINSICS

[CarolEidt]

I worry about FEATURE_SIMD_HW and FEATURE_HW_INTRINSICS, as they are basically a circular dependency. That is, due to the way the IsSupported flags are setup and ISAs are exposed, we couldn't have one without the other for most cases (at least for x86/x64).

The only thing (and it probably doesn't matter) is ISA families that have no SIMD (e.g. popcnt and lzcnt). I wouldn't expect FEATURE_SIMD_HW to require FEATURE_HW_INTRINSICS; only those ISAs that require it.

[CarolEidt]

How about?

FEATURE_SIMD_ABSTRACT -> FEATURE_SIMD_NUMERICS
FEATURE_SIMD_HW -> FEATURE_SIMD_HW_INTRINSICS

Sounds fine to me

[tannergooding]

Not clear why.

At least for x86/x64 (updated comment above to clarify that), we only have a couple of ISAs (such as Lzcnt and Popcnt) which expose just non SIMD intrinsics. The others (such as SSE) expose a mix of both SIMD (such as addps or sqrtss) and non-SIMD instructions (such as prefetch or sfence).

The only thing (and it probably doesn't matter) is ISA families that have no SIMD (e.g. popcnt and lzcnt). I wouldn't expect FEATURE_SIMD_HW to require FEATURE_HW_INTRINSICS; only those ISAs that require it.

Ok, so your suggesting FEATURE_SIMD_HW_INTRINSICS would cover the SIMD hardware intrinsics (any that take Vector64<T> or Vector128<T>, for example) and FEATURE_HW_INTRINSICS would only cover the intrinsics which don't take/return a SIMD type. And the ISAs which expose both would be disabled if both flags were not set. Is that correct?

[CarolEidt]

Ok, so your suggesting FEATURE_SIMD_HW_INTRINSICS would cover the SIMD hardware intrinsics (any that take Vector64 or Vector128, for example) and FEATURE_HW_INTRINSICS would only cover the intrinsics which don't take/return a SIMD type. And the ISAs which expose both would be disabled if both flags were not set. Is that correct?

Right - though I'm not adamant about the need to separate them, given that it's only those two, and it's unlikely we'd support those and not others.

[tannergooding]

given that it's only those two

I think ARM may be better suited to take advantage of this, given their ISA split/design, if we wanted to do it this way. @sdmaclea, could you confirm?

[sdmaclea]

advantage of this
@sdmaclea, could you confirm?

I would expect SIMD to be typically in separate classes from scalar operations. Although I do know why extra feature flags would be an advantage.

Personally the only reason for the FEATURE_SIMD_NUMERICS flag would be because eventually we want to disable and implement the operations in C# using HW intrinsics. So it would just be a discipline to keep unrelated code separate.

I don't see a reason to split FEATURE_HW_INTRINSICS and FEATURE_SIMD_HW_INTRINSICS. We can disable ISAs as needed....

[tannergooding]

After going through the majority of the code a couple times, looking at what is shared between SimdNumerics/HWIntrinsics, and going over the discussions made here; I propose the following changes:

• Move the existing SIMD Numerics code to use NamedIntrinsics (rather than SIMDIntrinsicIDs)
• Refactor GenTreeJitIntrinsic, GenTreeSIMD, and GenTreeHWIntrinsic
  • GenTreeJitIntrinsic should carry the NamedIntrinsic field (and not carry any SIMD specific info)
    • This would be generally available and not behind a feature flag
  • Rename GenTreeSIMD to GenTreeSimdIntrinsic, update it to carry gtSimdBaseType
    • I don't think we need gtSimdSize, as it was used to differentiate scalar intrinsics
    • gtType carries the actual TYP_SIMD*
    • This would be under the FEATURE_SIMD feature
  • Remove GenTreeHWIntrinsic entirely
    • Both SimdNumerics and HWIntrinsics will use GenTreeSimdIntrinsic
    • Scalar HWIntrinsics would just use GenTreeJitIntrinsic directly
• Refactor FEATURE_SIMD to cover TYP_SIMD* and related code
  • This includes merging core handling that is currently shared or duplicated between SimdNumerics/HWIntrinsics, such as:
    • The range checking code (GT_SIMD_CHK and GT_HW_INTRINSIC_CHK)
    • The isSIMD checks
    • The logic for not promoting TYP_SIMD*
    • etc
• Create a new feature FEATURE_SIMD_NUMERICS, this will cover the code specific to System.Numerics.Vector*
• Refactor the FEATURE_HW_INTRINSICS feature, this currently covers code specific to System.Runtime.Intrinsics but also currently duplicates some code

If FEATURE_SIMD is not defined, FEATURE_SIMD_NUMERICS and FEATURE_HW_INTRINSICS will be disabled.
FEATURE_SIMD_NUMERICS and FEATURE_HW_INTRINSICS will not be dependent on eachother and shouldn't share or duplicate code
FEATURE_SIMD will not have an external control flag. FEATURE_SIMD_NUMERICS and FEATURE_HW_INTRINSICS will (as would any other features that build on FEATURE_SIMD).