[Wasm RyuJIT]: Fix LIR Semantics in Stackifier Output

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures

[adamperlin]

This is a fix for an issue that came up in #126778, and is probably easiest to explain with a motivating example.

Consider the following case, where NOMOVE is a gentree operation we aren't allowed to move.

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... NOMOVE OP
t1 = ... OP
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (target)
CALL

The stackifier will first introduce a store to put t0 after t1:

t2 = ... NOMOVE OP
t3 = ... OP
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3 (arg1)
* t2 (arg2)
* t1 (arg3)
* t0 (call target)
CALL

And then recursively stackify the new STORE to tmp0, since it is a dataflow root.
The stackifier then marks tmp0 as free here, since it IS free in linear data flow order. Then, when the next operands to the call are
stackified, the stackifier introduces a temporary again, but reuses t0
because we freed it.

t2 = ... OP
+** STORE_LCL_VAR tmp0
t3 = ... OP
t2 = LCL_VAR tmp0
t0 = ... OP
+** STORE_LCL_VAR tmp0
t1 = ... OP
t0 = LCL_VAR tmp0
* t3
* t2
* t1
* t0 (target)
CALL

This produces invalid LIR; there is a store to tmp0 before one of its reads (t2) is consumed.

The simplest fix is to not release temporaries for reuse until all operands of a root tree have been processed. This PR adds a free list of temps which is recycled after each root gen tree has been processed, so we won't end up with any interference between temporaries while processing gentree ops which share a parent node.

[Copilot]

Pull request overview

Fixes a Wasm RyuJIT stackifier correctness issue where temporaries introduced during stackification could be released and then reused too early (within the same root tree), producing invalid LIR due to store/read interference.

Changes:

• Introduce a “pending release” bitset to defer releasing stackifier temporaries until a full root tree finishes processing.
• Replace immediate temporary release with AddTemporariesForPendingRelease + RemovePendingTemporaries at the end of root processing.
• Add dynamic growth logic for the pending-release bitset capacity.

[dotnet-policy-service]

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

[AndyAyersMS]

This seems plausible.

@SingleAccretion please take a look.

[SingleAccretion]

It is unfortunate we have to compromise CQ a bit to retain this LIR invariant, even though it doesn't correspond to codegen constraints (stack operands really are used at the LIR position, unlike register operands).

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

[adamperlin]

But I wonder if we can simplify the fix to just do:

if (initialDepth == 0)
ReleaseAllTemps();

I. e. only release the temporaries at statement boundaries.

I do think this approach would work and this would remove the need for tracking, so I'm going to give this approach a try.

Have you thought about what a "precise" fix would look like? A temporary can be used if it doesn't have refs between the current 'prev' position and 'use's parent. Tracking the parent on the stack is easy enough, tracking 'busy' temps considering the shifting position of both 'prev' and 'parent' seems trickier.

I haven't given this much thought since it seemed tricky to get right as you mention! I do think this would be nice to have if it turns out not to be too difficult. If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

[SingleAccretion]

If you have any thoughts on how we might do this efficiently, I'd definitely be interested!

No, not really. It seems it would require quite careful tracking for what in the end is still going to be a suboptimal result.

[Copilot]

Pull request overview

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

Comments suppressed due to low confidence (1)

src/coreclr/jit/lowerwasm.cpp:604

• This change addresses a subtle LIR correctness issue in the wasm stackifier; it would be good to add a targeted regression test (likely under src/tests/JIT/Regression) that produces a call with a non-movable target/arg ordering similar to the motivating example, and validates the method compiles/runs correctly under wasm RyuJIT. As-is, the fix is not protected against future refactors.

 GenTree* StackifyTree(GenTree* root)
{
int initialDepth = m_stack.Height();
// Simple greedy algorithm working backwards. The invariant is that the stack top must be placed right next
// to (in normal linear order - before) the node we last stackified.
m_stack.Push(&root);
GenTree* lastStackified = root->gtNext;
while (m_stack.Height() != initialDepth)
{
GenTree** use = m_stack.Pop();
GenTree* node = *use;
GenTree* prev = (lastStackified != nullptr) ? lastStackified->gtPrev : root;
while (node != prev)
{
// Maybe this is an intervening void-equivalent node that we can also just stackify.
if (IsDataFlowRoot(prev))
{
prev = StackifyTree(prev);
continue;
}
// At this point, we'll have to modify the IR in some way. In general, these cases should be quite
// rare, introduced in lowering only. All HIR-induced cases (such as from "gtSetEvalOrder") should
// instead be ifdef-ed out for WASM.
INDEBUG(const char* reason);
if (CanMoveForward(node DEBUGARG(&reason)))
{
MoveForward(node, prev DEBUGARG(reason));
}
else
{
node = ReplaceWithTemporary(use, prev);
}
m_anyChanges = true;

[adamperlin]

Stackifier temps used per block when compiling System.Private.CoreLib (with PEP calling convention changes from #126778)

This includes the out-of-stack-order call setup from #126778 to stress the stackifier.

Temps Used	Frequency	Percentage	Cumulative %
0	98,152	39.2%	39.2%
1	114,120	45.6%	84.8%
2	31,105	12.4%	97.3%
3	2,536	1.0%	98.3%
4	4,092	1.6%	99.9%
5	230	0.1%	~100.0%
6	4	~0.0%	~100.0%

Mean: 0.81 | Std Dev: 0.83 | Median (P50): 1

P10	P25	P50	P75	P90	P95	P99
0	0	1	1	2	2	4

99% of SPC methods need <= 4 temps for stackifying, most are using < 3, so I'm not too concerned about the releasing all temps after we process each root gentree in the stackifier. If we were to track them in a bit vector, we'd need at most 6 temp tracking slots to support all these samples.

[Copilot]

Pull request overview

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

[Copilot]

Pull request overview

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

[adamperlin]

@SingleAccretion I've now switched to the free/in use temp list approach that you suggested! I'd appreciate another review when you have a moment.

[SingleAccretion]

LGTM modulo one comment.

[Copilot]

Pull request overview

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

[adamperlin]

/ba-g Infrastructure failures