Disable VerifyFloatToUInt and VerifyFloatToNullableUInt tests

[EgorBo]

Disables two tests, see #47374.

Minimal repro for MSVC x86: #47374 (comment)

Godbolt with other compilers: https://godbolt.org/z/W74hTh

[dotnet-issue-labeler]

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

[benaadams]

/cc @tannergooding as its really annoying this failing in CI

[tannergooding]

Do we have tests covering these scenarios to ensure they are consistent across both current and future compilers?

[tannergooding]

Is the same not needed for float?

[saucecontrol]

float->uint casts are forced through double on x86

runtime/src/coreclr/jit/morph.cpp

Lines 175 to 180 in fdc8148

	// x86: src = float, dst = uint32/int64/uint64 or overflow conversion.
	&& (tree->gtOverflow() || varTypeIsLong(dstType) || (dstType == TYP_UINT))
	#endif
	)
	{
	oper = gtNewCastNode(TYP_DOUBLE, oper, false, TYP_DOUBLE);

[tannergooding]

Ah I see. I wonder if that's holdover from when we still used the x87 FPU stack

[tannergooding]

Should we log a bug on MSVC to see if this is something they want to fix in a future update, or if they need to keep the behavior for backcompat?

[SingleAccretion]

After giving this some more thought, I am wondering if the best long-term solution would be to just disable folding for constants that invoke implementation-specified behavior. This should kill a few birds with one stone:

1. We'll get cross-compilation consistency "for free". Today, as far as I can see, cross-compiling from x64 to arm64 will result in differences between the folding logic and the runtime logic (this is the same issue that Roslyn had).
2. We'll be shielded from compilers changing the unspecified behavior.
3. We'll not have to worry about harmonizing the codegen with the folding logic and ensuring they produce the same results (no need for maintaining tables of (Type <- Type)(Arch) -> results combinations and similar).

This is already the behavior for non-finite constants. @tannergooding what do you think?

(To be clear, this is for a future PR, this fix to unblock CI looks good modulo the fact that I think we should check for d <= TheLargestValueThatDoesNotOverflow, or even just 0 <= d && <= (double)uint.MaxValue, to be on the safe side. E. g. (unsigned)3.40282e+35 also exhibits this behavior on MSVC x86 as per my testing).

[EgorBo]

@tannergooding my goal was to unblock the CI asap and fix those two failing tests.

I was planning to add more test cases separately (e.g. these dotnet/roslyn#37527) to list all the edge cases/differences

[tannergooding]

@EgorBo, I think it's likely better to disable the two tests right now and work on getting a fix up separately.

As @SingleAccretion mentioned, this impacts more than just the value being handled in the PR and it would likely be good to get some validation tests up simultaneously.

[tannergooding]

@tannergooding what do you think?

I think having a temporary fix, such as deciding to not fold (or at least not fold when in R2R scenarios) is fine. But I think, long term, we would be in a better place by having these cases handled and well known, just like Roslyn did.

no need for maintaining tables of (Type <- Type)(Arch) -> results combinations and similar

I'm not super concerned with this. We will already need the first half of the table to know which values are edge cases and it would be good (IMO) to have some minor documentation on what values users can expect for these values and therefore how they will differ.

This is already the behavior for non-finite constants

This is a drawback, IMO. At least for floating-point, the IEEE 754 spec allows a certain amount of flexibility with handling of things like NaN without it being considered a "value changing optimization".
We aren't taking advantage of this and likely aren't generating the best code in certain scenarios. It's likely an edge case overall, but its still something most compilers handle.

[EgorBo]

@tannergooding makes sense, I've disabled the tests.

[benaadams]

@EgorBo might want to take "Fixes" out of summary so it doesn't close the other issue (or reopen after merge)

[hoyosjs]

Merging to unblock CI.

[akoeplinger]

Can we backport this to the release/6.0-preview1 branch? I saw these tests failing there too.