JIT: preserve side effect ordering when unrolling UTF16 comparisons

[EgorBo]

impUtf16StringComparison and impUtf16SpanComparison append the temp stores for the unrolled comparison with CHECK_SPILL_NONE, so expressions still sitting on the importer's evaluation stack below the call arguments were evaluated after them. For obj.Equals("cns") / StartsWith / EndsWith the unrolled expansion dereferences the receiver directly, so its NullReferenceException was raised ahead of earlier side effects.

Spill the side effects of the stack entries below the arguments before appending. Only those entries are spilled, so the already-captured argument trees stay valid.

Fixes #133966

[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]

🟡 Changes recommended

Unresolved critical and moderate findings require fixes and additional regression coverage.

Get a fresh assessment by requesting another Copilot review.

Pull request overview

Fixes JIT evaluation-order regressions when unrolling UTF-16 string and span comparisons.

Changes:

• Spills earlier side effects before appending unrolled comparison trees.
• Adds regression coverage for exception ordering and comparison behavior.

File summaries

File	Summary
src/tests/JIT/Regression_ro_2/Runtime_133966.cs	Adds regression tests for string comparison ordering.
src/coreclr/jit/importervectorization.cpp	Critical (1 vote): Spill GTF_ORDER_SIDEEFF entries as well as regular side effects. Moderate (3 votes): Add span-comparison coverage for exception ordering.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

[Copilot]

🔵 Needs a closer look

String and span paths still fail to spill stack entries containing only GTF_ORDER_SIDEEFF.

Review details

Suppressed comments (2)

src/coreclr/jit/importervectorization.cpp:523

• impSpillSideEffects(true, ...) still does not spill an evaluation-stack tree whose only ordering flag is GTF_ORDER_SIDEEFF: impSpillSideEffect selects GTF_GLOB_EFFECT for spillGlobEffects=true, while GTF_ORDER_SIDEEFF is excluded from that mask. Such a tree below the popped arguments remains on the stack, so the following CHECK_SPILL_NONE stores can still move this comparison ahead of the required ordering dependency. Please extend or use a spill path that also handles GTF_ORDER_SIDEEFF.

        impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("unrolled UTF16 string comparison"));

src/coreclr/jit/importervectorization.cpp:695

• The analogous span path has the same gap: impSpillSideEffects(true, ...) only selects GTF_GLOB_EFFECT, so a stack entry carrying only GTF_ORDER_SIDEEFF is left in place. The subsequent CHECK_SPILL_NONE stores can therefore still overtake that ordering dependency during span comparison expansion. Please extend or use a spill path that also handles GTF_ORDER_SIDEEFF.

        impSpillSideEffects(true, CHECK_SPILL_ALL DEBUGARG("unrolled UTF16 span comparison"));

• Files reviewed: 2/2 changed files
• Comments generated: 0 new
• Review effort level: Lite

[jakobbotsch]

LGTM, but seems the existing impStoreToTemp could instead pass CHECK_SPILL_ALL to benefit from normal interference checks

[EgorBo]

/ba-g known mono failure