Implement CONV_OVF_xxx in the Interpreter

[kg]

• Implemented most possible forms of CONV_OVF_xxx opcodes in native C++
• Moved the existing CONV_xxx opcodes that do float->int conversions into native C++ as well
• Refactored uses of INTERP_INDIRECT_HELPER_TAG to use a helper to strip the tag in one place instead of duplicating the tag logic everywhere
• Added new InterpOp type for possibly indirect helper pointers

[BrzVlad]

Any reason why this doesn't add support for all ovf opcodes ? Seems like we could do them all in one go since they are very similar anyway.

[kg]

Any reason why this doesn't add support for all ovf opcodes ? Seems like we could do them all in one go since they are very similar anyway.

No particular reason. If you want I'll add them all, I'll need to do some refactoring to reduce the bloat though.

[janvorli]

@kg, yes, it would be great if you could add all of them.

[kg]

Will add them all and re-request review once I'm done.

[kg]

OK, I think I've updated everything and made it correct/fairly clean now. Thanks for your patience.

[janvorli]

LGTM modulo the comment, thank you!

[jkotas]

Calling the managed helper like this looks both complicated and slow.

Given that you are promoting the value to double first anyway, I think it would be faster and simpler to do something like this:

// Generic version that works for 32-bit and smaller TResult
template <typename TResult, typename TSource> static void ConvOvfFpHelper(int8_t *stack)
{
    // First, promote the source value to double
    double promoted_src = LOCAL_VAR(ip[2], TSource);

    // checking for overflow as int64_t is easier since int64_t has higher precision than double
    if ((promoted_src != promoted_src) || (promoted_src < (double)std::numeric_limits<int64_t>::min()) || (promoted_src >= (double)std::numeric_limits<int64_t>::max())
    {
        COMPlusThrow(kOverflowException);
    }

    TResult result = (int64_t)result;
    if ((result < std::numeric_limits<TResult>::min()) || (result > std::numeric_limits<TResult>::max())
    {
        COMPlusThrow(kOverflowException);
    }

    LOCAL_VAR(ip[1], int32_t) = (int32_t)result;
}

// Specialization for int64_t
template<> static void ConvOvfFpHelper<int64_t, TSource>(int8_t *stack, const int32_t *ip)
{
    // First, promote the source value to double
    double promoted_src = LOCAL_VAR(ip[2], TSource);

    if ((promoted_src != promoted_src) || (promoted_src < (double)std::numeric_limits<int64_t>::min()) || (promoted_src >= (double)std::numeric_limits<int64_t>::max())
    {
        COMPlusThrow(kOverflowException);
    }

    LOCAL_VAR(ip[1], int64_t) = (int64_t)result;
}

// Specialization for uint64_t
template<> static void ConvOvfFpHelper<uint64_t, TSource>(int8_t *stack, const int32_t *ip)
{
    // First, promote the source value to double
    double promoted_src = LOCAL_VAR(ip[2], TSource);

    if ((promoted_src != promoted_src) || (promoted_src <= -1) || (promoted_src >= (double)std::numeric_limits<uint64_t>::max())
    {
        COMPlusThrow(kOverflowException);
    }

    LOCAL_VAR(ip[1], uint64_t) = (uint64_t)result;
}

[kg]

OK, I'm willing to do the work to duplicate the managed logic like this instead of reuse it. I was under the impression we were trying to avoid adding more duplication in the interpreter at this stage, but this probably isn't worse in practice than calling the managed helpers since it's so complicated to call them.

[kg]

Re: your other comment (github won't let me reply to it), it should be straightforward to make the helper accept a 'throw or return 0' flag and use it for the other fp conv opcodes.

[jkotas]

I was under the impression we were trying to avoid adding more duplication in the interpreter at this stage

I think we should treat it case by case and strike a reasonable balance.

We have the logic duplicated in number of places currently. For example, INTOP_MUL_OVF_I8 is inlined - but calling the JIT helper with the same name would work too

it should be straightforward to make the helper accept a 'throw or return 0' flag and use it for the other fp conv opcodes.

I would introduce a separate method for the non-throwing helpers. They do not always return 0 - they should saturate the value.

[jkotas]

For example, the expression for the ->uint64_t non-throwing conversion can be:

return (promoted_src != promoted_src) ? 0 : (promoted_src <= -1) ? 0 : (promoted_src >= (double)std::numeric_limits<uint64_t>::max()) ? std::numeric_limits<uint64_t>::max() : (double)promoted_src;

[kg]

the <= -1 is because we're in round-towards-zero mode, i assume?

[jkotas]

Right

And >= (double)std::numeric_limits<uint64_t>::max() is to deal with rounding to the nearest value for integer -> double conversions. (Notice that it is >= and not > that would be more intuitive.)

[kg]

I would have missed the >= vs > thing if I hadn't paid attention, thanks for pointing it out so I can be certain. That one's surprising but it makes sense given that it will round to the nearest larger value in some cases.

FWIW the way the managed helpers implement this stuff is very different from your suggestion, but I'm fine with being different as long as what I've written works.

[jkotas]

You can copy the constants and comparisons from the managed helpers if you prefer. I will make it easier to see that the two implementations are in sync.

I have written my suggestion this way to use std:: constants.

[kg]

I also noticed that the numeric limit we want here is ::lowest, not ::min. I'm glad I read the documentation more carefully.

[kg]

Revised again to hopefully address all feedback. I tried to implement the conversions without type specializations to reduce duplication, but I might have gotten that wrong, in which case I'll do it manually with specializations.

[jkotas]

Suggested change

	outOfRange = (src != src) || (src <= -1) || (src >= maxValue);
	outOfRange = (src != src) || (src <= -1) || (src > maxValue);

I think it should be > for unsigned types given how to values round up and down. (You can do a quick test for the last convertible value and first overflowing value to verify.)

Alternatively, you can copy to constants and exact logic from the managed helpers as discussed in the other thread.

[jkotas]

Suggested change

	else if (src < minValue)
	else if (!std::numeric_limits<TResult>::is_signed && (src < minValue))

?

I am not sure whether the compiler is smart enough to optimize out the unnecessary float comparison

[kg]

For:

    if (std::numeric_limits<TResult>::is_signed)
        outOfRange = (src != src) || (src < minValue) || (src >= maxValue);

causes an exception when converting 32767.0 to short.

[jkotas]

Oh, I have missed that the latest version is doing the floating-point overflow check against the TResult range (my suggestion was to check int64_t range first, convert, and the check the integer result again for the smaller TRange).

It is certainly possible and faster to do just one check against the TResult, just need to pick the right limits.

For example, the upper limit for double->short conversion needs to be src >= 32768.0) so that 32767.99999 passes the check (it will be rounded towards zero to 32767).

[jkotas]

Mono appears to just do it with pre-computed magic constants for i64/u64

This is the exact copy of the CoreCLR managed impl (

runtime/src/libraries/System.Private.CoreLib/src/System/Math.cs

Lines 1691 to 1719 in 5f3e1ff

	[StackTraceHidden]
	internal static long ConvertToInt64Checked(double value)
	{
	const double two63 = Int32MaxValueOffset * UInt32MaxValueOffset;
	// Note that this expression also works properly for val = NaN case
	// We need to compare with the very next double to two63. 0x402 is epsilon to get us there.
	if (value is > -two63 - 0x402 and < two63)
	{
	return double.ConvertToIntegerNative<long>(value);
	}
	ThrowHelper.ThrowOverflowException();
	return 0;
	}
	[StackTraceHidden]
	internal static ulong ConvertToUInt64Checked(double value)
	{
	const double two64 = UInt32MaxValueOffset * UInt32MaxValueOffset;
	// Note that this expression also works properly for val = NaN case
	if (value is > -1.0 and < two64)
	{
	return double.ConvertToIntegerNative<ulong>(value);
	}
	ThrowHelper.ThrowOverflowException();
	return 0;
	}

)

if (val > ((double)G_MININT32 - 1) && val < ((double)G_MAXINT32 + 1))

This looks correct to me. Where are the results different?

[kg]

Mono appears to just do it with pre-computed magic constants for i64/u64

This is the exact copy of the CoreCLR managed impl (

runtime/src/libraries/System.Private.CoreLib/src/System/Math.cs

Lines 1691 to 1719 in 5f3e1ff

	[StackTraceHidden]
	internal static long ConvertToInt64Checked(double value)
	{
	const double two63 = Int32MaxValueOffset * UInt32MaxValueOffset;
	// Note that this expression also works properly for val = NaN case
	// We need to compare with the very next double to two63. 0x402 is epsilon to get us there.
	if (value is > -two63 - 0x402 and < two63)
	{
	return double.ConvertToIntegerNative<long>(value);
	}
	ThrowHelper.ThrowOverflowException();
	return 0;
	}
	[StackTraceHidden]
	internal static ulong ConvertToUInt64Checked(double value)
	{
	const double two64 = UInt32MaxValueOffset * UInt32MaxValueOffset;
	// Note that this expression also works properly for val = NaN case
	if (value is > -1.0 and < two64)
	{
	return double.ConvertToIntegerNative<ulong>(value);
	}
	ThrowHelper.ThrowOverflowException();
	return 0;
	}

)

if (val > ((double)G_MININT32 - 1) && val < ((double)G_MAXINT32 + 1))

This looks correct to me. Where are the results different?

I must have run my tests incorrectly in VS. I'll figure out what went wrong on monday. Thanks for the help so far.

[kg]

I think what's wrong here is that I ran my tests for rounding (boundary point of 0.5) instead of truncation. Will update the PR once I've run new tests.

[Copilot]

Pull Request Overview

This PR implements various CONV_OVF_xxx opcodes in the interpreter and moves several float‐to‐int conversion operations to native C++ while refactoring indirect helper pointer handling.

• Added new helper functions for conversion (both checked and unchecked) in interpexec.cpp.
• Updated test cases in Interpreter.cs to validate the new conversion opcodes.
• Modified intops.def and compiler.cpp to support the new opcodes and helper pointer resolution.

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated no comments.

Show a summary per file

File	Description
src/tests/JIT/interpreter/Interpreter.cs	Added tests to validate new CONV_OVF and related conversion opcodes.
src/coreclr/vm/interpexec.cpp	Introduced helper templates and conversion functions for overflow conversions.
src/coreclr/interpreter/intops.h	Added new enum value to represent helper function pointers.
src/coreclr/interpreter/intops.def	Registered the new CONV_OVF opcodes with updated op types.
src/coreclr/interpreter/compiler.cpp	Updated opcode emission to support the new conversion opcodes; added a TODO to reuse helper data items.

Comments suppressed due to low confidence (2)

src/coreclr/interpreter/compiler.cpp:2125

• [nitpick] Consider expanding the TODO comment with more details or a plan for how future reuse of the helper data item index will be implemented. This could help improve clarity for future maintainers.

    // Interpreter-TODO: Find an existing data item index for this helper if possible and reuse it

src/coreclr/interpreter/compiler.cpp:3753

• [nitpick] Review and document the choice of conversion opcode in non-64-bit mode for StackTypeI4 in the CEE_CONV_OVF_U case. Clarifying this uncertainty according to the specification would help future developers understand the rationale.

                    // FIXME: Is this the right conv opcode?

[jkotas]

Thank you!