search

dotnet / runtime

.NET is a cross-platform runtime for cloud, mobile, desktop, and IoT apps.
https://docs.microsoft.com/dotnet/core/
MIT License
15.17k stars 4.72k forks source link

Intel Hardware Intrinsics test framework design deficiences #10349

Open 4creators opened 6 years ago

4creators commented 6 years ago

A while ago we have had a long discussion about the scope of testing which should be done during Intel Hardware Intrinsics development (for details see https://github.com/dotnet/coreclr/pull/15771).

There were two proposals put forward:

  1. Test as little as possible concentrating on a check if expected assembly instruction is emitted;
  2. Test for wider range of values including known problematic ones

Currently we use approach from point 1 which IMO opinion is deficient since it led to several missed implementation bugs i.e.:

https://github.com/dotnet/coreclr/issues/17957 https://github.com/dotnet/coreclr/issues/18039 https://github.com/dotnet/coreclr/issues/18041 https://github.com/dotnet/coreclr/issues/18043

It seems that using small set of problematic values for each numeric type would solve that problem without need for creating complex test scenarios i.e.:

1) for integral types: min, max, -1, 0, 1 2) for unsigned integral types: min, max, 1 3) for floating types: +/- infinity, +/- max/min, +/- 0, subnormal

@AndyAyersMS @CarolEidt @fiigii @sdmaclea @tannergooding

category:testing theme:hardware-intrinsics skill-level:intermediate cost:small

tannergooding commented 6 years ago

Currently we use approach from point 1 which IMO opinion is deficient since it led to several missed implementation bugs i.e.:

I don't agree.

dotnet/runtime#10312 is a case where the exposed API did not correctly expose the semantics of the underlying hardware instruction. The API needs to be fixed and tests would likely not have caught this as it requires a particular coding convention to hit in the first place (you need to have the JIT sign extend the result, without the code being optimized away).

dotnet/runtime#10348 is not related to hardware intrinsics, but is instead related to the existing SIMD intrinsics.

dotnet/runtime#10344 is because the are no tests covering that code in the first place.

dotnet/runtime#10346 is also because there is no test covering the code. The existing Sse2\ConvertToInt32WithTruncation test is actually calling ConvertToInt32

fiigii commented 6 years ago

I agree with @tannergooding that the above issues not related to the test framework or test values.

tannergooding commented 6 years ago

NOTE: That isn't to say that we shouldn't add additional regression tests for the cases that we are hitting and that we shouldn't do additional reviews of the API surfaces to ensure that other APIs won't hit similar issues.