Update CoreCLR interpreter float->int saturation to match JIT #116834
Conversation
|
Tagging subscribers to this area: @BrzVlad, @kotlarmilos |
There was a problem hiding this comment.
Pull Request Overview
This PR updates the CoreCLR interpreter’s floating-point→integer conversion to use a saturating conversion via an intermediate type and adjusts the existing test so that it fails when the interpreter behavior does not match RyuJIT.
- Invert the test assertion in
TestConvBoundariesfor sub-Int32 conversions. - Generalize
ConvFpHelperto take an intermediate integer type (TIntermediate) and update all call sites accordingly.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| src/tests/JIT/interpreter/Interpreter.cs | Invert the if check on (a, b) and add a comment referencing the JIT mismatch issue |
| src/coreclr/vm/interpexec.cpp | Change ConvFpHelper signature to accept TIntermediate and modify its logic and call sites |
Comments suppressed due to low confidence (1)
src/coreclr/vm/interpexec.cpp:172
- Consider adding unit tests for the new 64-bit conversion paths (e.g.,
INTOP_CONV_I8_R4,INTOP_CONV_U8_R8) to ensure theTIntermediate-based saturation logic behaves correctly for large result types.
template <typename TResult, typename TIntermediate, typename TSource> static void ConvFpHelper(int8_t *stack, const int32_t *ip)
| @@ -1540,8 +1540,11 @@ public static bool TestConvBoundaries (double inRangeShort, double outOfRangeSho | |||
| int c = (int)inRangeInt, | |||
| d = (int)outOfRangeInt; | |||
|
|
|||
| if (a != b) | |||
| // See https://github.com/dotnet/runtime/issues/116823 - they should *not* currently match if target size is smaller than int32 | |||
| // if (a != b) | |||
There was a problem hiding this comment.
[nitpick] Remove the commented-out condition or add a // TODO explaining when and why it will be reverted; leaving the old if (a != b) in comments may confuse future maintainers.
| // if (a != b) | |
| // TODO: The condition `if (a != b)` was commented out due to the behavior described in issue #116823. | |
| // Revisit this condition if the issue is resolved and the behavior changes. |
Copilot uses AI. Check for mistakes.
See #116823
The test for this was causing outerloop failures because the interpreter's expected behavior does not match ryujit. The best course of action right now is to make the interpreter match ryujit so the test can be re-enabled on outerloop.