VectorNNN<T>.Sqrt missing test coverage and unspecified behavior

[kg]

The Sqrt API for VectorNNN<T> does not specify its behavior for unusual inputs, and it also doesn't appear to have tests in the System.Runtime.Intrinsics test suite. It's not clear to me what should happen if (for example) you attempt to call Vector128<Int64>.Sqrt(Vector128<Int64>.AllBitsSet), as the benchmarks do. Based on reading the Scalar implementation, we appear to cast the T to Double before passing it to Math.Sqrt, then cast it back to T. So the behavior I would expect is:

For each input,

1. Cast the input (an Int64 with the value -1) to Double, yielding a Double with the value -1.0
2. Pass the Double -1.0 to System.Math.Sqrt, yielding a Double with a NaN value
3. Convert the return value back to Int64, yielding (??? an exception?)

According to the official docs VectorNNN<T>.Sqrt does not throw any exceptions other than NotSupportedException (we also don't specify Math.Sqrt(double) as throwing any exceptions, but that's reasonable unlike a possible integer Sqrt - something we don't offer - which might have to throw).

So to summarize some issues I see:

• It's not clearly specified what Vector Sqrt does when passed integer values that have no square root, at least not one that can be represented in an integer
• The rounding behavior is also not specified when the value's square root is not exactly representable in a T and needs to be rounded/truncated
• It's not specified what happens if the result of the operation is too big to fit in T
• We don't have test coverage for VectorNNN<T>.Sqrt and might be missing it for other key methods
• We don't have proper benchmark coverage for Sqrt and may be missing it for other methods (there is already an issue about the relevant benchmarks in the performance repo)

For most parts of the vector API you can just presume it will do what the scalar version would do, but there's no scalar equivalent here. Even reading the implementation merely tells me what it probably does, it doesn't tell me what it's meant to do.

Do we have a general documentation page for vectors that specifies how integer vectors behave? I wasn't able to find an overview like this, but I admit I didn't search for too long (since I only visited the docs when trying to puzzle this out).

[ghost]

Tagging subscribers to this area: @dotnet/area-system-runtime-intrinsics See info in area-owners.md if you want to be subscribed.

Issue Details

---

The `Sqrt` API for `VectorNNN` does not specify its behavior for unusual inputs, and it also doesn't appear to have tests in the System.Runtime.Intrinsics test suite. It's not clear to me what should happen if (for example) you attempt to call `Vector128.Sqrt(Vector128.AllBitsSet)`, as the benchmarks do. Based on reading the `Scalar` implementation, we appear to cast the T to Double before passing it to `Math.Sqrt`, then cast it back to T. So the behavior I would expect is: For each input, 1. Cast the input (an Int64 with the value `-1`) to Double, yielding a Double with the value `-1.0` 2. Pass the Double `-1.0` to `System.Math.Sqrt`, yielding a Double with a `NaN` value 3. Convert the return value back to Int64, yielding (??? an exception?) According to the official docs `VectorNNN.Sqrt` does not throw any exceptions other than `NotSupportedException` (we also don't specify `Math.Sqrt(double)` as throwing any exceptions, but that's reasonable unlike a possible integer `Sqrt` - something we don't offer - which might have to throw). So to summarize some issues I see: * It's not clearly specified what Vector `Sqrt` does when passed integer values that have no square root, at least not one that can be represented in an integer * The rounding behavior is also not specified when the value's square root is not exactly representable in a T and needs to be rounded/truncated * It's not specified what happens if the result of the operation is too big to fit in T * We don't have test coverage for `VectorNNN.Sqrt` and might be missing it for other key methods * We don't have proper benchmark coverage for `Sqrt` and may be missing it for other methods (there is already an issue about the relevant benchmarks in the performance repo) For most parts of the vector API you can just presume it will do what the scalar version would do, but there's no scalar equivalent here. Even reading the implementation merely tells me what it probably does, it doesn't tell me what it's meant to do. Do we have a general documentation page for vectors that specifies how integer vectors behave? I wasn't able to find an overview like this, but I admit I didn't search for too long (since I only visited the docs when trying to puzzle this out).

Author:	kg
Assignees:	-
Labels:	`documentation`, `area-System.Runtime.Intrinsics`, `increase-code-coverage`
Milestone:	-

[tannergooding]

Tests exist, they are generated from templates and cover testing correctness: https://raw.githubusercontent.com/dotnet/runtime/main/src/tests/Common/GenerateHWIntrinsicTests

The conversion from long->double->long matches the general case of ConvertToDouble(VectorXXX<Int64>) and ConvertToInt64(VectorXXX<double>) which likewise has coverage and testing.

In general, NaN->integer is considered implementation defined behavior and the exact result is non-deterministic, even for scalar results.

Sqrt for integers is really a case that likely shouldn't have been exposed/supported. It should have instead been like Ceiling/Floor where it only exists for floating-point values. However, that's not how the original Vector<T> APIs were designed in 2014 and so the handling was brought forward for consistency.

[kg]

Do the HW intrinsic tests get run for targets without HW vector intrinsics?

[tannergooding]

Yes. Arm32 and Unix x86 have no support.

Mono (and therefore WASM) are still onboarding SIMD support.

We also stress test all levels of HWIntrinsics disabled (that is we individually test disabling each available ISA in the hierarchy, including disabling HWIntrinsics entirely). Such disablement is available to consumers as well via the DOTNET_Enable*=0 environment variables (where * is the name of the ISA, for example DOTNET_EnableSSE3=0 disables SSE3 and later ISAs that derive from SSE3.

[kg]

This is extremely valuable context for digging into the possible failures I'm looking at, thank you!

[tannergooding]

@kg does this need to stay open given the above or is there still a concern on your end about missing coverage for some scenario?

[ghost]

This issue has been marked needs-author-action and may be missing some important information.

[kg]

I'm still uncertain whether we actually have test coverage that would catch the issue I ran into, but we won't be able to find out any time soon since the related stuff (SIMD intrinsics in the interpreter + jiterpreter) hasn't landed yet. I'm ok with closing it and then reopening later if it turns out we do need more tests.

[tannergooding]

Will close this for the time being. Feel free to re-open if we do have edge cases that need additional coverage.