Fixing the codegen for x64 intrinsics to use EA_8BYTE where needed.

[tannergooding]

This resolves https://github.com/dotnet/coreclr/issues/23438

CC. @CarolEidt, @fiigii

[tannergooding]

Just a note: I'm not a fan of doing this via SpecialCodeGen. I'm sure we could do this better via flags or normal checks, but this was the quickest/smallest fix to ensure these were correct.

[fiigii]

LGTM.

[CarolEidt]

@tannergooding - it appears that these intrinsics are covered by the automated tests; is the problem that those tests didn't cover a sufficient range of values?

[CarolEidt]

LGTM

[tannergooding]

it appears that these intrinsics are covered by the automated tests; is the problem that those tests didn't cover a sufficient range of values?

@CarolEidt, kind-of. The RNG for float/double produces a value between 0.0 and 1.0 (as that is what Random.GetNextDouble does), so it will never hit this issue.

We also don't have anything that guarantees the Int64/UInt64 returned will be greater than int.MaxValue. The current implementation gets a set of 8 bytes and combines them together, so it is possible some tests don't get a value that would properly test these cases.

• We previously agreed to also use a fixed-seed; so we also can't catch this by repeated outer-loop testing and have to add explicit tests instead.

[tannergooding]

We could probably add a method that gets a float/double from the full range of finite floats by generating an int32/int64 in the desired range and then bit-casting that to float/double. But you still have the problem of ensuring that it validates a value greater than int.MaxValue where that is important.

[CarolEidt]

We could do something similar to what I did in VectorConvert.cs: https://github.com/dotnet/coreclr/blob/master/tests/src/JIT/SIMD/VectorConvert.cs#L85. It's a bit clunky, but it starts with a set of corner cases, and then adds random values that are transformed to cover a somewhat broader range.

[tannergooding]

then adds random values that are transformed to cover a somewhat broader range.

If we want to have methods that return a float in a broader range, we should just do:

float GetFloat(float minValue, float maxValue)
{
    Debug.Assert(!float.IsNaN(minValue) && !float.IsNaN(maxValue));
    return Random.Next(BitConverter.SingleToInt32Bits(minValue), BitConverter.SingleToInt32Bits(maxValue));
}

As the above will generate a value in the full range given and ensures it is evenly distributed.

But realistically, we should probably just add explicit tests for anything where the instruction encoding is dependent on the operand size. Relying on random inputs is just going to be prone to failure as long as those random inputs are actually static due to a fixed seed (as it means testing will never actually expose the failures if they exist).

[CarolEidt]

we should probably just add explicit tests for anything where the instruction encoding is dependent on the operand size. Relying on random inputs is just going to be prone to failure as long as those random inputs are actually static due to a fixed seed (as it means testing will never actually expose the failures if they exist).

I think that's what I was getting at - that we should have a mechanism for including explicit values that exercise the corner/edge cases, with additional "random" values.

And I persist in my belief that it is unhelpful to have truly random failures in a test system. Even if the tests spit out their seed value, it is problematic to reproduce these, especially when (as is often the case) one doesn't have all the output from the testing.

[tannergooding]

I think that's what I was getting at - that we should have a mechanism for including explicit values that exercise the corner/edge cases, with additional "random" values.

The test generator I setup has most of this available already. The only bad part is it currently requires you to re-declare most of the metadata when you really just want to the value generator field to be an array instead. It probably wouldn't be too difficult to rewrite.

And I persist in my belief that it is unhelpful to have truly random failures in a test system.

And I still hold that this is mostly an infrastructure problem with the existing tests and could be remedied. But like last time, it's not a hill I'm willing to fight on for the CoreFX/CoreCLR repos and I'm fine with keeping the current mechanism 😄.

[tannergooding]

I've logged https://github.com/dotnet/coreclr/issues/23465 to track updating the test generation script to support multiple input values so that way metadata duplication can be reduced when adding new edge cases to be tested.