[lewing]

CSE rejects a use when its exception set has items that the defs' exception sets don't. That is correct when the def might not have proven the exception impossible. But a def that has executed rules out more than its VN says:

• Each indirection it evaluates proves its base address is non-null.
• Each integer division proves its divisor is nonzero (and that it did not overflow).

That holds even when an earlier phase marked the node non-faulting and VN therefore left the exception out.

This change adds optValnumCSE_ImpliedExcSet, which collects these items for a def's own tree, and uses it in two places:

• optValnumCSE_Availability: defs count their own liberal exception set plus the items implied by their tree. As before, defExcSetCurrent is the intersection over all defs, and later defs check earlier promises against the same set.
• The Index-phase "better candidate with more exceptions" swap: it is skipped when the earlier occurrence's tree already covers the later one's extra exceptions.

The walks only run for candidates whose occurrences don't all have the same exception set (csdMixedExcSets, set in optValnumCSE_Index). Candidates whose occurrences all share one set are unaffected.

There are no VN, IR or IR flag changes, so loop hoisting and other users of VN exception sets are unaffected.

Where the mismatched exception sets come from

Measured with temporary metrics in optValnumCSE_Availability on main. Local SuperPMI, windows x64, FullOpts. Columns: libraries.pmi / libraries.crossgen2 / coreclr_tests.

rejected uses, by the exception the defs don't cover	pmi	crossgen2	coreclr_tests
all rejected uses	7,383	816	14,728
NullPtrExc only	2,814	815	2,534
... already proved by the def's own tree	2,697	705	2,476
InvalidCastExc	4,526	0	1,065
HelperOpaqueExc	32	0	11,004
IndexOutOfRangeExc	50	1	128

• NullPtrExc only. Most of these come from morph's local assertion prop. optNonNullAssertionProp_Ind marks one copy of a load non-faulting while another copy, for example the loop-test copy duplicated by loop inversion, stays faulting. Example: Microsoft.CodeAnalysis.BitVector:Invert, where the this._bits load is CSE'd only after global assertion prop, which runs too late.
• NullPtrExc, not covered here. A value is stored and then reloaded through a different address, e.g. tmp1.f = arr[i]; ... tmp1.f. The store that proves tmp1 non-null is outside the def's tree.
• InvalidCastExc and HelperOpaqueExc are benign. The rejected node is a CHKCAST* helper call, or a COMMA(CLASSPROFILE32 helper, x) in Instrumented Tier1 code. Each has the same normal VN as its operand, and the operand is accepted as a use, so no CSE is lost. This change does not target them.

Effect on the counts

	pmi	crossgen2	coreclr_tests
CSEs performed	174,117 → 175,221	138,622 → 138,966	435,841 → 436,989
null-only rejected uses	2,814 → 117	815 → 110	2,534 → 58
abandoned candidates	209 → 69	41 → 1	480 → 393

SuperPMI asmdiffs (local, windows x64, main → this PR)

collection	methods with diffs	smaller / larger	code size	PerfScore (sum)	geomean PerfScore of diffed methods
libraries.pmi	1,449	812 / 454	-148 B	-0.065%¹	-3.08%
libraries.crossgen2	453	238 / 179	+178 B	-0.022%	-3.27%
coreclr_tests	1,203	474 / 686	+10,951 B	-0.0002%	+1.30%

¹ Excludes contexts with PerfScore > 1e5; the four RegexGenerator methods around 2e6 also improve. Including them gives -0.71%.

• coreclr_tests regressions. Most of the PerfScore regressions in coreclr_tests are copies of a single method, one per test. These are the Instrumented Tier1 Dictionary<__Canon,__Canon>.FindValue (+32 B) and Remove (+23 B); TryInsert shrinks by 22 B.
• Cause. The newly allowed CSE of the method table load this->MT (mov r, [this], used for runtime lookups) lives across calls. That raises register pressure, so other locals spill. The crossgen2 regressions (OrderedDictionary, SegmentedDictionary) follow the same pattern. This is the usual tradeoff in the CSE heuristic, not a correctness problem.
• No asserts. The Checked diff JIT replays without asserts. The only failures are SuperPMI missing-data contexts (1 in pmi).
• Throughput. CI tpdiff for c53628c is +0.01% to +0.05% overall across all 9 targets (windows x64 +0.03% to +0.05%). CI asmdiffs for windows x64 currently include no benchmarks collection.

Relation to #136612

Relates to #136612. This follows up on @AndyAyersMS's review thread there: #136612 (comment).

#136612 forwards byref-exposed stores to same-address loads in VN. It also omits NullPtrExc from a forwarded load whose address the forwarding store already dereferenced, so that CSE accepts the load as a use.

This change does not cover that case. The CSE def there is the stored value, and the store that proves the address non-null is outside the def's tree. So #136612 keeps its VN NullPtrExc omission.

Note

Superseded: an earlier revision of this PR (before c53628c) also used what the def's whole statement rules out. That version made #136612's omission nearly redundant: dropping the omission left 9 / 59 / 19 small diffs, with unchanged codegen for Utf8JsonReader.SkipWhiteSpace on wasm and for the ByrefStoreForwarding test. Those measurements no longer apply, because the statement-level part was removed after review.

Note

This PR description was generated with the help of GitHub Copilot.

[azure-pipelines]

Azure Pipelines:
Successfully started running 5 pipeline(s).
10 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.

[NinoFloris]

You probably want to do this during value numbering instead of as a new walk.

My prompted attempt:
NinoFloris@fbf37f1

[lewing]

Thanks — I measured fbf37f1 against this PR on local SPMI (windows x64, base main 8237dfd; pmi / crossgen2 / coreclr_tests). Null-only rejected CSE uses go from 2,814 / 815 / 2,534 on main to 21 / 16 / 29 here vs 399 / 51 / 986 with the VN version: block-level dominance misses facts from non-dominating statements that CSE availability covers. Relative to this PR, the VN version is +6,097 / +3,949 / +12,246 bytes and PerfScore +0.030% / +0.009% / +0.135%. Changing the loads' VN exception sets also changes which occurrences CSE matches, e.g. NumericSortRect.NumHeapSort (Tier1-OSR) is +31% PerfScore because the hot-loop CSE #1 drops from 21 to 9 occurrences. There are also 52 extra SPMI misses in coreclr_tests. A VN-level approach is appealing because every consumer would see it (e.g. redundant branch opts' side-effect check), but it is broader and its results here are mixed, so I'd keep this change CSE-local and look at a VN version separately. The cost of the extra walk isn't measured yet; I'll get TP numbers.

Note

This reply was generated with the help of GitHub Copilot.

[NinoFloris]

Can you try again against https://github.com/NinoFloris/dotnet-runtime/tree/jit/vn-dominated-exceptions? It now handles the commas that likely caused the NumHeapSort regression, and I've added support for block joins so it's not just directly dominated checks.

A big reason why I'd rather do it during value numbering is because this walk is quadratic, and once you add a hashtable you are halfway to what the value numbering case does. Maybe it should still be confined to CSE only but that depends on how high the tp cost of doing it during value numbering is.

It's a good idea btw :)

[lewing]

Re-measured 2244f42 (same setup; pmi / crossgen2 / coreclr_tests). It's now competitive: null-only rejected uses 70 / 58 / 51 (this PR: 21 / 16 / 29; main: 2,814 / 815 / 2,534), and relative to this PR it's -2,891 / -1,133 / +1,167 bytes with PerfScore +0.003% / -0.001% / ~0. NumHeapSort no longer regresses and the extra SPMI misses are gone (2 in pmi). So codegen is roughly a wash and TP will decide.

CI tpdiff for this PR was +0.10% to +1.0% (FullOpts, depending on target) before f872555, which now only walks for candidates whose occurrences have differing exception sets (no codegen change locally). With it, tpdiff is +0.02% to +0.13% across all 9 targets (e.g. windows x64 +0.07% to +0.12%, linux arm64 +0.06% to +0.13%). Part of that remaining cost is the extra CSEs themselves.

On the quadratic concern: it's one walk per def plus one cached walk per statement, now limited to mixed candidates, so it's only superlinear for nested defs. If your VN version's TP comes in lower, I'm happy to go that way — could you open it as a PR so CI measures it?

Note

This reply was generated with the help of GitHub Copilot.

[AndyAyersMS]

Some nits -- how important is the "statement" level propagation? That seems like it's more risky / less useful?

[lewing]

/azp run runtime-coreclr superpmi-diffs

[azure-pipelines]

Azure Pipelines:
Successfully started running 1 pipeline(s).

[AndyAyersMS]

@jakobbotsch thoughts on this?

We should also reconcile with #137045. I'm not sure which is the preferred direction; here I steered Larry away from refining exception sets in VN. I feel like having VN create what essentially seem to be ORDER_SIDEEFF dependent VNs seems like a risk.

The other annoyance is local AP's weakness in dealing propagating into loops, which creates many of these inconsistent cases in the first place.

[NinoFloris]

Some examples from other compilers:

• LLVM rechecks safety at the new location using dominating checks and other known facts, rather than carrying an ordering flag on the operation itself. Out-of-band instead of on the IR, but obviously this would costs us a lot of tp.
• C2 and Graal encode these dependencies directly in their graphs (as expected), constraining reordering during opt until scheduling puts it in order. Not super relevant.
• V8 Turboshaft is probably the closest example: CanDependOnChecks marks operations that may rely on earlier checks. Its reordering rules keep them behind checks and branches. It looks much more like a decent end state of JIT: Centralize exception and write-barrier proofs in value numbering #137045.

It’s not surprising that centralizing the work pays off. Fitting it into RyuJIT is awkward because there’s zero(?) shared infrastructure for preserving contextual assumptions during code motion (GTF_ORDER_SIDEEFF already suffers from this). Each phase handles that ad hoc, which is where the actual risk is. #137045 is not really worse than in that respect, but it does expand that pattern…

[jakobbotsch]

This looks like a pretty conceptually simple change and improvement to CSE. I would be happy with taking this if it improves things and handles some of these cases.

#137045 looks like it is coming up with a new dependency/ordering framework within the JIT. While experimentation is interesting I think we would need to discuss what direction we want to take before we even get started on something like that. We have discussed that before and there are breadcrumbs in some places on GitHub.

The parts in #137045 that are storing facts about blocks in side tables and reasoning about those seems reasonable as long as it does not have to be shared across phases, but I am not sure that is going to scale all that well once IR transformations make it into the mix and you want to use this framework throughout the JIT.

My two cents: I would like a framework where ordering side effects of nodes can be classified at higher detail, similar to what OperExceptions does, and then some kind of partial ordering for the reasoning about the ordering effects. This information should be derivable from the IR, it should not be preserved in side tables. Phases that want to reason in more detail about ordering can compute the additional information they need with off-the-shelf helpers during their walks (similar to TreeLifeUpdater). Obviously it isn't clear how this would look either, certainly I would expect some engineering to be required as well.

[AndyAyersMS]

diffs show some size increases and in some cases more size regressions than improvements.

I don't think that's enough to hold this up, but wanted to mention it. It seems like our CSE heuristics likely need revisiting, as they struggle to balance prolog / call crossing costs vs benefits.

Perf score is improved so we can hope we're still generally making good choices.

[AndyAyersMS]

Let's take this.