Skip to content

[wasm][interp] Fix ConvertToInteger intrinsic on wasm #127220

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
radekdoulik wants to merge 2 commits into
base: main
Choose a base branch
from
Open

[wasm][interp] Fix ConvertToInteger intrinsic on wasm #127220

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8f59db6
[browser][interp] Fix ConvertToInteger intrinsic on wasm
radekdoulik Apr 20, 2026
be0853a
Merge branch 'main' into clr-wasm-s-r-tests-update
radekdoulik Apr 21, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions src/coreclr/interpreter/compiler.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3474,6 +3474,7 @@ bool InterpCompiler::EmitNamedIntrinsicCall(NamedIntrinsic ni, bool nonVirtualCa
m_pLastNewIns->SetDVar(m_pStackPointer[-1].var);
return true;

case NI_PRIMITIVE_ConvertToInteger:
case NI_PRIMITIVE_ConvertToIntegerNative:
{
Comment on lines +3477 to 3479
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This case now covers both ConvertToInteger and ConvertToIntegerNative, but the error text later in the block still says "ConvertToIntegerNative: source type is not floating point". Consider updating that diagnostic to mention both intrinsics (or use a generic "ConvertToInteger" message) so failures are not misleading.

Copilot uses AI. Check for mistakes.
CHECK_STACK(1);
Expand Down
7 changes: 7 additions & 0 deletions src/coreclr/interpreter/interpconfig.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,13 @@ void InterpConfigValues::Initialize(ICorJitHost* host)
#define RELEASE_CONFIG_INTEGER(name, key, defaultValue) m_##name = host->getIntConfigValue(key, defaultValue);
#include "interpconfigvalues.h"

#ifdef TARGET_WASM
// WASM-TODO: update when R2R is enabled
Copy link
Copy Markdown
Member

@jkotas jkotas Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is replacing one bug with a different bug. I do not think we want to do this. We are actively working on enabling R2R, so the folks working on that will need to be reverting this change all the time.

Also, this configuration is duplicated on the VM side. It is hard to reason about the behavior when the VM side and the interpter side do not agree about the mode that the interpreter is running in.

Copy link
Copy Markdown
Member Author

@radekdoulik radekdoulik Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to get some performance gain meanwhile, before we have the R2R intrinsics in place. What if I guard it by FEATURE_DYNAMIC_CODE_COMPILED instead of TARGET_WASM?

That would work better for folks working on R2R.

Copy link
Copy Markdown
Member

@jkotas jkotas Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was hoping to get some performance gain meanwhile

The intrinsics are implemented in the interpreter for correctness only, any performance improvement is accidental. I doubt that you would see a measurable performance gain.

What if I guard it by FEATURE_DYNAMIC_CODE_COMPILED instead of TARGET_WASM

I am not sure what you mean.

Copy link
Copy Markdown
Member

@BrzVlad BrzVlad Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't follow why we would need to tweak the interp mode. If we have an interpreter implementation for an intrinsic that is correct and fast, shouldn't we use it all the time, regardless of the interp mode ?

Copy link
Copy Markdown
Member

@jkotas jkotas Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some intrinsics do not have fully defined behavior. However, we do want to guarantee that the behavior is same within the process lifetime, so we either want to use interpreter implementation of the intrinsics all the time in given process; or R2R implementation of the intrinsics all the time in a given process, but we do not want to mix and match.

Copy link
Copy Markdown
Member

@BrzVlad BrzVlad Apr 22, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If that's the case, seems like it would mean that we need to avoid intrinsifying those methods in interpreter, given r2r code would be using that different behavior that we have no control over. So I would expect this PR to remove intrinsic support, to match JIT behavior, rather than add new one.

Anyhow, I don't think we should care about InterpMode=3 performance since it won't be used anywhere. I still don't understand how changing it here makes any difference. Since on no-JIT platforms we already interpret everything that the runtime asks us to (so methods that weren't already found in r2r).

// Default to InterpMode 3 unless explicitly overridden via DOTNET_InterpMode.
Copy link

Copilot AI Apr 21, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment says InterpMode defaults to 3 on WASM "unless explicitly overridden", but the implementation forces 0 -> 3 unconditionally. That means an explicit DOTNET_InterpMode=0 can’t be honored/distinguished from the unset default. If 0 should remain a valid explicit override, consider instead changing the default passed to getIntConfigValue for TARGET_WASM (e.g., defaultValue=3) or reword the comment to match the actual behavior.

Suggested change
// Default to InterpMode 3 unless explicitly overridden via DOTNET_InterpMode.
// On WASM, treat InterpMode 0 as InterpMode 3.

Copilot uses AI. Check for mistakes.
if (m_InterpMode == 0)
m_InterpMode = 3;
#endif

m_isInitialized = true;
}

Expand Down
2 changes: 2 additions & 0 deletions src/coreclr/interpreter/intrinsics.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,8 @@ NamedIntrinsic GetNamedIntrinsic(COMP_HANDLE compHnd, CORINFO_METHOD_HANDLE comp
{
if (!strcmp(methodName, "ConvertToIntegerNative"))
return NI_PRIMITIVE_ConvertToIntegerNative;
else if (!strcmp(methodName, "ConvertToInteger"))
return NI_PRIMITIVE_ConvertToInteger;
else if (!strcmp(methodName, "MultiplyAddEstimate"))
return NI_System_Math_MultiplyAddEstimate;
}
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Runtime/tests/System.Runtime.Tests/System/DoubleTests.GenericMath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,6 @@ public static void op_InequalityTest()

[Fact]
[SkipOnMono("https://github.com/dotnet/runtime/issues/100368")]
[ActiveIssue("https://github.com/dotnet/runtime/issues/123011", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsCoreCLR))]
public static void ConvertToIntegerTest()
{
// Signed Values
Expand Down
1 change: 0 additions & 1 deletion src/libraries/System.Runtime/tests/System.Runtime.Tests/System/SingleTests.GenericMath.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -346,7 +346,6 @@ public static void op_InequalityTest()

[Fact]
[SkipOnMono("https://github.com/dotnet/runtime/issues/100368")]
[ActiveIssue("https://github.com/dotnet/runtime/issues/123011", typeof(PlatformDetection), nameof(PlatformDetection.IsBrowser), nameof(PlatformDetection.IsCoreCLR))]
public static void ConvertToIntegerTest()
{
// Signed Values
Expand Down
Loading