[Wasm RyuJIT] Fixes for two codegen asserts by kg · Pull Request #126514 · dotnet/runtime

kg · 2026-04-03T20:14:17Z

Fixes BitConverter.ToBFloat16 and Vector`1.op_Multiply

dotnet-policy-service · 2026-04-03T20:15:15Z

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

Copilot

Pull request overview

Fixes two WebAssembly RyuJIT codegen assertion failures by handling edge cases around struct locals whose effective “register type” is not directly representable as TYP_STRUCT in wasm codegen.

Changes:

Ignore leftover GT_LCL_VAR nodes whose computed register type is TYP_UNDEF when they are marked as IsUnusedValue() (e.g., after block-store pruning in LIR).
Ensure GT_STORE_LCL_VAR for enregistered struct locals uses the struct layout’s register type when determining the local.set operand size.

src/coreclr/jit/codegenwasm.cpp

kg · 2026-04-03T21:49:47Z

@dotnet/jit-contrib PTAL

src/coreclr/jit/codegenwasm.cpp

Copilot

Pull request overview

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

src/coreclr/jit/liveness.cpp

kg · 2026-04-06T18:25:39Z

src/coreclr/jit/liveness.cpp

-                                }
-                            }
+                LclVarDsc& varDsc = m_compiler->lvaTable[lclAddr->AsLclVarCommon()->GetLclNum()];
+                isDeadStore = varDsc.lvTracked && !VarSetOps::IsMember(m_compiler, life, varDsc.lvVarIndex);


@jakobbotsch Do you have thoughts on how to do this correctly? Ideally we would call ComputeLifeLocal here to get the exact same true/false value as the old analysis, but doing that will introduce liveness tracking bugs because ComputeLifeLocal will get called again later on the same node. Should I rework ComputeLifeLocal so it has a read-only mode that can be used here?

For more context the problem is that on wasm we get the LIR order LCL_ADDR, LCL_VAR, STORE_BLK while other targets have LCL_VAR, LCL_ADDR, STORE_BLK. This means that the existing approach of pruning the block store from inside of the lcl_addr doesn't work anymore - it leaves a stray lcl_var behind.

it leaves a stray lcl_var behind.

Why is that a problem? As a principle the backends should handle unused values whether or not they get removed by this liveness pass.

the lcl_var is a struct, so we have no way to push it onto the evaluation stack. I think Andy said that other backends don't like this either (stray lcl_vars that don't fit into a register)

It sounds like WASM is not marking the LCL_VAR as contained then. I think other backends will do that in lowering for STORE_BLK and I would expect that to make their respective codegens handle the stray LCL_VAR fine.

It sounds like WASM is not marking the LCL_VAR as contained then. I think other backends will do that in lowering for STORE_BLK and I would expect that to make their respective codegens handle the stray LCL_VAR fine.

We don't have special handling for stay unused TYP_STRUCT locals in codegen (see e. g.

runtime/src/coreclr/jit/codegenxarch.cpp

Line 5081 in 1c60926

GetEmitter()->emitIns_R_S(ins_Load(type, m_compiler->isSIMDTypeLocalAligned(tree->GetLclNum())),

).

I don't think it would be especially useful to make that (TYP_STRUCT local legal if unused) a new contract, since it won't really solve the problem with WASM where we can't get away with treating LCL_ADDR as the location of the def.

For me the question here is what do we need to do to support async's use of liveness, since for LIR purposes it would be sufficient to only handle assorted 'blessed' IR shapes for indirect stores to tracked (or promoted) locals.

Thanks for diving into this, you two. I think what I'll do is make sure we handle unused nodes properly in codegen + teach liveness to remove the stray LCL_VAR when it removes the STORE_BLK.

I can't remove the stray lcl_var trivially without breaking liveness, will keep digging into it but I'd kind of prefer to just move on and fix that part later, if that's okay with you two?

I think I found a way to do this that doesn't break anything, updated the PR.

It seems like an "ad-hoc" kind of limitation if we phrase it the they way we'be been looking at it here. I think it's justifiable if we consider it to just be a subset of the "no TYP_STRUCT values allowed in IR post-lower" (modulo the multi-reg special case) rule that has deeper reasons for existing than just codegen not having some ifs.

My general opinion is that it should be possible to remove IR with BlockRange().Remove(node, true), without having to depend on some optional pass (like liveness) to come in and clean up after you. Otherwise we are making it too hard to manipulate our own IR.

My general opinion is that it should be possible to remove IR with BlockRange().Remove(node, true), without having to depend on some optional pass (like liveness) to come in and clean up after you. Otherwise we are making it too hard to manipulate our own IR.

It is a tradeoff, isn't it. Without first-class struct values after lowering, you either get this restriction we have with struct values today, or you need to contort codegen to support these struct values, with the assumption they're unused. The latter seems like a muddier contract to me.

There is a somewhat separate question of whether it is a good restriction to have before lowering. I can see it not being the case and lowering being responsible for transforming such nodes (as it already is partially today).

…]:op_Multiply caused by dead store removal

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

src/coreclr/jit/codegenwasm.cpp

jakobbotsch · 2026-04-09T11:16:43Z

src/coreclr/jit/liveness.cpp

                                {
                                    Lowering::TransformUnusedIndirection(data->AsIndir(), m_compiler, block);
                                }
+                                else if (data == mostRecentLocalVarOrField)


You can use LIR::LastNode here and remove mostRecentLocalVarOrField to make this work robustly.

But I am surprised you need this at all. What was the problem with just removing stray local nodes as the data unconditionally?

It would explode when visiting the removed node a second time and trying to remove it a second time. I worked around that by preventing it from removing it twice, but then got random crashes in the x64 emitter.

That sounds odd. How did we end up visiting the removed node? We shouldn't see it since it has been removed.

That didn't make sense to me either. I checked and its gtPrev and gtNext were both nullptr since it had been removed, so I think removing it was leaving a dangling reference elsewhere somehow?

You can use LIR::LastNode here and remove mostRecentLocalVarOrField to make this work robustly.

I've been looking at this and it's not clear how I would substitute LIR::LastNode for my variable here. Is it guaranteed that the range will only contain the store and its dependencies?

That didn't make sense to me either. I checked and its gtPrev and gtNext were both nullptr since it had been removed, so I think removing it was leaving a dangling reference elsewhere somehow?

I looked over the code again and realized it's because ComputeLifeLIR does its iteration custom so it was iterating over dead nodes. This seems like a defect in how it's designed and could potentially happen for any node we remove inside of this method. I found a workaround, so let me know if you think what I did is acceptable. I'm willing to rework the method to not be vulnerable to this.

Copilot

Pull request overview

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

src/coreclr/jit/codegenwasm.cpp

Copilot · 2026-04-10T20:21:06Z

src/coreclr/jit/liveness.cpp

+                                        next = data->gtPrev;
+                                    if (end == data)
+                                        end = data->gtNext;


The end == data check looks unreachable here: end is initialized to firstNode->gtPrev (a node outside the block’s range), while data is a node inside the range. Removing this branch (and adding braces for the two if statements for consistency with surrounding code) would reduce confusion and make the iterator-adjustment logic clearer.

Suggested change

next = data->gtPrev;

if (end == data)

end = data->gtNext;

{

next = data->gtPrev;

}

kg added the arch-wasm WebAssembly architecture label Apr 3, 2026

Copilot AI review requested due to automatic review settings April 3, 2026 20:14

kg added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Apr 3, 2026

dotnet-policy-service bot assigned kg Apr 3, 2026

Copilot started reviewing on behalf of kg April 3, 2026 20:15 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

src/coreclr/jit/codegenwasm.cpp Outdated Show resolved Hide resolved

kg marked this pull request as ready for review April 3, 2026 21:49

SingleAccretion reviewed Apr 3, 2026

View reviewed changes

src/coreclr/jit/codegenwasm.cpp Outdated Show resolved Hide resolved

Copilot AI review requested due to automatic review settings April 3, 2026 23:22

Copilot started reviewing on behalf of kg April 3, 2026 23:23 View session

Copilot AI reviewed Apr 3, 2026

View reviewed changes

src/coreclr/jit/liveness.cpp Show resolved Hide resolved

src/coreclr/jit/liveness.cpp Outdated Show resolved Hide resolved

kg marked this pull request as draft April 3, 2026 23:36

build-analysis bot mentioned this pull request Apr 4, 2026

System.Runtime.Tests SIGSEGV crash on ARM64 Linux (azurelinux.3.arm64) #126516

Closed

kg commented Apr 6, 2026

View reviewed changes

kg added 6 commits April 8, 2026 14:41

Fix assert while compiling BitConverter.ToBFloat16

4ff2109

Fix for assert in codegen for System.Numerics.Vector`1[System.__Canon…

8e3f886

…]:op_Multiply caused by dead store removal

jit-format

19173af

Address PR feedback

0866b8f

Remove the dead lcl_fld/lcl_var

3a805a3

jit-format

50cde09

Copilot AI review requested due to automatic review settings April 8, 2026 23:18

kg force-pushed the wasm-emit-nyi branch from 2126523 to 50cde09 Compare April 8, 2026 23:18

Copilot started reviewing on behalf of kg April 8, 2026 23:19 View session

kg marked this pull request as ready for review April 8, 2026 23:21

kg requested review from SingleAccretion and jakobbotsch April 8, 2026 23:21

Copilot AI reviewed Apr 8, 2026

View reviewed changes

src/coreclr/jit/codegenwasm.cpp Show resolved Hide resolved

build-analysis bot mentioned this pull request Apr 9, 2026

The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#3008

Open

3 tasks

This was referenced Apr 9, 2026

[android-arm64] The Operation will be canceled. The next steps may not contain expected logs. dotnet/dnceng#6408

Open

System.Net.NameResolution.Tests DNS failures: Name or service not known #126641

Open

jakobbotsch reviewed Apr 9, 2026

View reviewed changes

kg added 2 commits April 10, 2026 13:11

Implement without local var

3ad27ef

jit-format

65c5e5d

Copilot AI review requested due to automatic review settings April 10, 2026 20:15

Copilot started reviewing on behalf of kg April 10, 2026 20:16 View session

Copilot AI reviewed Apr 10, 2026

View reviewed changes

kg added 2 commits April 10, 2026 13:23

Fix incorrect comment; use Delete instead of Remove like other code does

6034e07

Address copilot feedback

36c9866

Conversation

kg commented Apr 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dotnet-policy-service bot commented Apr 3, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

kg commented Apr 3, 2026

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SingleAccretion Apr 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jakobbotsch Apr 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Apr 10, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

kg commented Apr 3, 2026 •

edited

Loading

jakobbotsch Apr 7, 2026 •

edited

Loading

SingleAccretion Apr 7, 2026 •

edited

Loading

jakobbotsch Apr 9, 2026 •

edited

Loading