[RyuJIT Wasm] Memops on wasm are not containable

[kg]

No description provided.

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[Copilot]

Pull request overview

This PR removes an incorrect NYI_WASM assertion from the WasmRegAlloc::isContainableMemoryOp method. The function's implementation—returning false to indicate that memory operations are not containable on WebAssembly—was already complete. The NYI_WASM macro would throw a not-yet-implemented error when TARGET_WASM is defined, preventing the function from executing even though it was correctly implemented.

• Removes blocking NYI_WASM assertion from a fully implemented function
• Allows memory operation containability checks to execute correctly on WebAssembly

[SingleAccretion]

What is the call stack that hits this? We shouldn't be trying to contain any memory ops, not yet. Though we will eventually do this for casts (to emit things like CAST<long>(IND<int>) as one load).

If this is hit from a caller that "makes sense", the right implementation would be to move what is now in lsrabuild.cpp to regalloc.cpp.

[kg]

If you run a superpmi replay on x86 right now it gets hit ~40 times, see #122627 - I reproduced it locally too

[kg]

cc @dotnet/jit-contrib
SingleAccretion points out that we may want to contain indirections later, so maybe this is wrong?

[AndyAyersMS]

@kg can you paste the call stack where you see this?

[kg]

I don't know how to get stacks out of spmi, but here are a few of the asserts:

[14:56:31] ISSUE: <ASSERT> #6145 Z:\runtime\src\coreclr\jit\regallocwasm.cpp (51) - Assertion failed 'NYI_WASM: isContainableMemoryOp' in 'System.Single:MinMagnitudeNumber(float,float):float' during 'Lowering nodeinfo' (IL size 46; hash 0x387b13cc; FullOpts)
[14:56:31] ISSUE: <ASSERT> #14594 Z:\runtime\src\coreclr\jit\regallocwasm.cpp (51) - Assertion failed 'NYI_WASM: isContainableMemoryOp' in 'System.IO.UnmanagedMemoryAccessor:Write(long,float):this' during 'Lowering nodeinfo' (IL size 14; hash 0x82f6430c; FullOpts)
[14:56:32] ISSUE: <ASSERT> #3684 Z:\runtime\src\coreclr\jit\regallocwasm.cpp (51) - Assertion failed 'NYI_WASM: isContainableMemoryOp' in 'System.BitConverter:Int64BitsToDouble(long):double' during 'Lowering nodeinfo' (IL size 7; hash 0x8bea83fc; FullOpts)
[14:56:32] ISSUE: <ASSERT> #28900 Z:\runtime\src\coreclr\jit\regallocwasm.cpp (51) - Assertion failed 'NYI_WASM: isContainableMemoryOp' in 'System.Globalization.CultureData:GetLocaleName(System.String,byref):bool' during 'Lowering nodeinfo' (IL size 38; hash 0xc29d82d7; FullOpts)

[kg]

I think this is one of them:

[SingleAccretion]

ContainCheckBitCast

So this is one of those cases that "makes sense" (though it does mean we will need to handle it in bitcast codegen). That implies the second option (moving the existing impl from lsrabuild.cpp).

[AndyAyersMS]

ContainCheckBitCast

So this is one of those cases that "makes sense" (though it does mean we will need to handle it in bitcast codegen). That implies the second option (moving the existing impl from lsrabuild.cpp).

Do you mean the logic in checkContainedOrCandidateLclVar?

[SingleAccretion]

Do you mean the logic in checkContainedOrCandidateLclVar?

No, just the part that's called by lowering: isContainableMemoryOp.

Edit: we don't handle contained bitcasts in codegen yet. But when we start to, we can certainly add a version of what checkContainedOrCandidateLclVar does as well I suppose.