Preserve operand execution order during JIT morphing

[tannergooding]

Morph must visit operands in execution order: assertions established by a later-executing operand cannot be used to optimize an earlier one. The HW intrinsic walk used physical operand order, allowing a null check to be removed before a side-effecting call. Ordinary binary-node traversal and several morph rewrites also failed to preserve reversed operand ordering.

This change:

• Morphs simple nodes, long-multiply operands, and HW intrinsic operands in execution order, and processes early call arguments before late arguments on remorph. QMARK/COLON assertion handling is preserved.
• Explicitly sequences reversed array-index and arithmetic-helper operands, makes comma-throw propagation order-aware, and clears reverse flags when converting to operators that cannot carry them.
• Checks operand-swapping negation rewrites in execution order and preserves the relevant ordering through addition reassociation, shift-to-mask rewriting, and rotate recognition.

Adds compact regressions for the original concatenation failure and the SIMD double-negation subtraction exception-order failure at 128/256/512 bits, plus basic scalar coverage. The scalar controls pass on the original JIT; they are not claimed as reproductions of the reversed-IR failures.

Validation

• Checked x64/x86 and Release x64 JIT builds; JIT formatting passed.
• Final x64 regression runner: 351 passed, 3 skipped, both normally and under JitStress2.
• Hardware-intrinsic runner: 2,551 passed. All 14 targeted cases passed normally, under JitStress2, and with MinOpts; the eight SIMD/ordinary-node cases also passed with AVX2 disabled and with hardware intrinsics disabled.
• The concatenation and SIMD subtraction cases failed before their respective fixes. Additional reversed-tree cases were validated with temporary IR probes; natural C# reachability was not established for every corrected path. x86 was build-only; other architectures were not executed.

Independent of #133776; no changes from that PR are required.

Resolves #134334

Note

This PR description and implementation were prepared with GitHub Copilot assistance.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
11 pipeline(s) were filtered out due to trigger conditions.
There may be pipelines that require an authorized user to comment /azp run to run.

[dotnet-policy-service]

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

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical operand-ordering issue remains, and several modified paths lack focused regression coverage.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1 · 1

Open (2)

• Clear reverse flag after swapping operands · New
• Add regressions for untested ordering rewrites · New

What changed in this PR

This PR preserves operand execution order during JIT morphing and adds regression coverage for ordering-related failures.

Changes:

• Updates morphing and rewrite paths to respect execution order.
• Adds sequencing and reverse-flag handling for affected transformations.
• Adds SIMD and scalar regression tests.

Review findings:

• Critical (1 vote): morph.cpp rewrites at lines 7979, 9712, 10335, and 10732 may retain GTF_REVERSE_OPS after swapping operands, reversing side effects or exceptions.
• Moderate (2 votes): Tests do not cover several other changed ordering paths.

File	Summary
src/​tests/​JIT/​Regression_ro_2/​Runtime_134334.cs	Adds regression coverage for operand-order failures.
src/​coreclr/​jit/​morph.cpp	Preserves execution order across morphing and optimization rewrites.

[EgorBo]

I'd probably also extend debug diagnostics in fgdiagnose (e.g. to catch stale GTF_REVERSE on things that shouldn't have it), but not necessary for this PR.

LGTM, but I wonder if @jakobbotsch or @dotnet/jit-contrib want to take a look

[tannergooding]

I'd probably also extend debug diagnostics in fgdiagnose (e.g. to catch stale GTF_REVERSE on things that shouldn't have it), but not necessary for this PR.

Agreed and I'd prefer we do that for the this assumes firstOp is not null after swap comment too. We shouldn't be annotating things that are functionally unary ops as being reversed as that's liable to lead to problems throughout the JIT.

[tannergooding]

@lewing, found this issue in Mono from the regression test. Essentially Mono would sometimes drop the nullcheck for a callvirt in some cases when it tried to convert it to a direct call instead.

[Copilot]

Copilot review overview

🟡 Changes recommended

A critical rewrite may retain reverse-order metadata and reorder side effects.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Clear reverse-op flag for nonzero constant subtraction rewrites · New

Resolved since last review (2)

• Clear reverse flag after swapping operands
• Add regressions for untested ordering rewrites

[jakobbotsch]

Ordinary binary-node traversal and several morph rewrites also failed to preserve reversed operand ordering.

For some context: for a long time we never set GTF_REVERSE_OPS anywhere before morph, hence why this was never a problem. I can see that some SIMD paths are setting it, but I don't think we ever discussed whether we want to expand our support and broaden the use of the flag. Personally I am not so sure, this flag is a quite gnarly part of the IR.

Various other places in the JIT would not handle the reverse order correctly either, e.g. local morph and the way it associates extra information with operands nodes does not work with this flag either. So even with this PR I do not think we can just start setting GTF_REVERSE_OPS before morph on arbitrary nodes.

If we were to try to support this everywhere then I think it would warrant a stress mode where the importer reorders operands and sets the flag. The code patterns of this PR are quite complex and with zero testing of those paths I am not very convinced about the correctness.

Would it be possible to limit this PR to the nodes that can set the flag before morph today? I.e. the SIMD nodes? I agree with Egor it would be nice to expand fgDebugCheckFlagsAndTypes and we could have an OperSupportsReverseOpEvalOrderBeforeMorph for the new list that was also checked there.

[tannergooding]

for a long time we never set GTF_REVERSE_OPS anywhere before morph, hence why this was never a problem.

Notably we do end up setting it in some cases, from prepping call argument costs in morph itself, and so remorph risks encountering it already. Likewise it can be hit from any post costing phase that recalls morph (like early prop or assertion prop do). Ideally none of these are impactful, but I'm not convinced they actually are correct as is.

Would it be possible to limit this PR to the nodes that can set the flag before morph today?

I can limit it to the SIMD cases -or- I guess I could go and remove its usage from all the pre-import SIMD cases too?

Really its whatever you think is the best long term here.

I personally expect that it would be better to support reverse_ops more broadly long term. As it reduces some of the complexity around commas, avoids additional nodes being introduced/tracked, and is something many areas already need to consider (ordering and side effects), so using the ordering traversal more consistently helps push us towards that and allow more robust improvements to the codebase later.

The code patterns of this PR are quite complex and with zero testing of those paths I am not very convinced about the correctness.

I am interested as to what you find complex about them in particular? Most of the changes are SIMD related already with a couple for the helper operands and fgMorphSmpOp where its explicitly using evaluation order. That is primarily just replacing direct op1/op2 usage with a firstOp/secondOp that is swapped based on whether isReverseOps is set; and corresponding regression tests were added for those scenarios.

[jakobbotsch]

for a long time we never set GTF_REVERSE_OPS anywhere before morph, hence why this was never a problem.

Notably we do end up setting it in some cases, from prepping call argument costs in morph itself, and so remorph risks encountering it already. Likewise it can be hit from any post costing phase that recalls morph (like early prop or assertion prop do). Ideally none of these are impactful, but I'm not convinced they actually are correct as is.

I think the primary thing depending on execution order is local assertion prop, which we only run during global morph. So that's probably why it's not been as problematic. FWIW, I would also like us to stop calling morph after morph. Recent work on gtFoldExpr is probably a good step towards that.

Would it be possible to limit this PR to the nodes that can set the flag before morph today?

I can limit it to the SIMD cases -or- I guess I could go and remove its usage from all the pre-import SIMD cases too?

Really its whatever you think is the best long term here.

I personally expect that it would be better to support reverse_ops more broadly long term. As it reduces some of the complexity around commas, avoids additional nodes being introduced/tracked, and is something many areas already need to consider (ordering and side effects), so using the ordering traversal more consistently helps push us towards that and allow more robust improvements to the codebase later.

If we can remove it from pre-import SIMD cases that would IMO be best. We can avoid having to teach the frontend about the possibility of the flag and keep that as the invariant for the future.

In my opinion a better design would be new genTreeOps for the reverse opers when they are truly beneficial to have in HIR, instead of the broad support for all opers. For example, there should not be any practical reason or need to support GTF_REVERSE_OPS on a GT_ADD. OTOH, it may be beneficial to have a reversed version of GT_SUB (I don't know if it actually is, but as an example). It has downsides too though, like some foldings may need to learn about the reverse versions.

The reverse versions could be HIR-only and rationalization would normalize them away.

The main source of GTF_REVERSE_OPS is (or used to be) gtSetEvalOrder. But I think the fact that we do that evaluation on tree-based IR is just a remnant of the past. If we were implementing that today it might make more sense to do it during rationalization or directly on LIR.

The code patterns of this PR are quite complex and with zero testing of those paths I am not very convinced about the correctness.

I am interested as to what you find complex about them in particular? Most of the changes are SIMD related already with a couple for the helper operands and fgMorphSmpOp where its explicitly using evaluation order. That is primarily just replacing direct op1/op2 usage with a firstOp/secondOp that is swapped based on whether isReverseOps is set; and corresponding regression tests were added for those scenarios.

The flag handling and how GTF_REVERSE_OPS gets inherited from certain sub-operands in some of the transformation does not exactly look trivial to me.

[tannergooding]

Thanks. Will look at removing the reverse_ops from the simd instead and get a separate PR up.