Repository navigation
Conversation
…ering Stores through unknown addresses were modeled as opaque heap mutations, so a load from the address just stored to always got a fresh value number. Record a VNF_ByrefExposedStore memory state for non-volatile int/long/float/double stores (long only where it is not decomposed) and give a following load of the same type from the same liberal address the stored value. The forwarded load records its loop memory dependence so LICM cannot hoist it above the store. When the conservative address also matches, a load that could fault is marked nonfaulting with an ordering side effect. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
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. |
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch |
|
SuperPMI results from the CI run:
Few methods change: whole-collection PerfScore moves by −0.002% to −0.04%. Browser wasm has a single 89K-method crossgen2 library collection, so its coverage is thin. arm64: the reload is gone. Repeated and w2, w2, #0xD1FFAB1E
str w2, [x1]
mov x1, x0
- ldr w2, [x1]
and w2, w2, #0xD1FFAB1E
str w2, [x1]x64/x86: the main source of size regressions. In the same method, x64 loses its memory-operand read-modify-write forms because the forwarded value is CSE'd into a register (52 → 66 bytes). PerfScore goes from 22.5 to 14.0, but that model overstates the cost of RMW instructions, so I'd treat this as roughly neutral at best: - and dword ptr [rax], 0xD1FFAB1E
+ mov edx, dword ptr [rax]
+ and edx, 0xD1FFAB1E
+ mov dword ptr [rax], edx
mov rax, rcx
- and dword ptr [rax], 0xD1FFAB1E
+ and edx, 0xD1FFAB1E
+ mov dword ptr [rax], edxI could stop forwarding when the reload only feeds a store back to the same address. arm32: Thumb-2 encoding growth and register pressure. Most arm32 regressions use the same or fewer instructions but grow by 2–10 bytes: the extra live value moves operands into high registers, which need 32-bit encodings. A few are real loop costs, because the forwarded value has to stay live across the loop. In - ldr r1, [r4+0x0C]
- cmp r1, 0
+ cmp r10, 0
beq SHORT G_M52454_IG08
...
- subs r1, r2, 1
+ add r10, r2, -1
+ mov r1, r10
str r0, [r4+0x08]
str r1, [r4+0x0C]This also adds a callee-saved register and 12 bytes of frame. wasm. In local.get 0
i32.load 0 20 ;; V02
- local.get 0
- i32.load 0 20 ;; V02
- i32.load 0 0
+ local.get 9
i32.const -64
i32.or
i32.store 0 0The byref itself ( No replay failures or asserts. A handful of contexts per platform (≤12) are missing with the diff JIT because the new JIT asks for data the old collection didn't record. I haven't measured end-to-end performance; the only timing I have is the isolated Note This comment was prepared with GitHub Copilot from the CI SuperPMI summaries. |
|
/azp run runtime-coreclr jitstress, runtime-coreclr libraries-jitstress, runtime-coreclr outerloop |
|
Azure Pipelines: Successfully started running 3 pipeline(s). |
On xarch, lowering emits "op [mem], x" when a store's value is an RMW-capable operator applied to a load from the same address. Forwarding the previous store's value to that load let CSE keep the value in a register and lost the memory-operand form, growing chains like "v &= a; v &= b" on x64/x86. Don't forward to a load that is the direct source of such an update. Loads that read the updated value afterwards are still forwarded. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
/azp run runtime-coreclr superpmi-diffs |
|
Azure Pipelines: Successfully started running 1 pipeline(s). |
|
Updated SuperPMI results after the second commit, which stops forwarding into xarch read-modify-write updates (CI run, full collection sets for the current JIT-EE GUID):
PerfScore improves overall in every collection. There are no replay failures. A few contexts per platform are missing with the diff JIT (≤3, 17 on windows x86) because the new JIT asks for data the collections didn't record. xarch read-modify-write. On x64/x86, a load that is the direct source of
This isn't a clean A/B because main and the collections moved between runs. A local A/B on the three windows x64 collections with base, before and after JITs built from the same main commit shows the gate alone: 173 changed methods, 166 smaller and 5 larger, −801 B. arm64 and wasm codegen is unchanged by the gate. The remaining x64/x86 growth is mostly a read after an update now coming from a register. For example: - add dword ptr [rcx], edx
- cmp dword ptr [rcx], edx
+ mov eax, edx
+ add eax, dword ptr [rcx]
+ mov dword ptr [rcx], eax
+ cmp eax, edxThat's one fewer memory read for about 4 bytes. Also skipping these updates would save a further ~1 KB on windows x64 but gives back about half of the PerfScore gain on the library collections, so I left it. Stress runs on the first commit: Note This comment was prepared with GitHub Copilot from the CI SuperPMI summaries and local SuperPMI runs. |
Instead of marking a forwarded load nonfaulting during value numbering, leave its flags alone and omit the null check exception from its VN when the store that is the current memory state dereferenced the same conservative address. This removes the statement side-effect refresh from fgValueNumberBlock. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…tates differ Also forward TYP_REF and TYP_BYREF stores. When GcHeap and ByrefExposed memory are tracked separately, keep the store as the ByrefExposed state instead of making it opaque; direct stores to address-exposed locals still replace it. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Replace the xarch check in value numbering that stopped forwarding a store to the source load of "*p = *p op x" with a screen that runs after CSE candidates are located. The load is removed from its candidate unless the op's value has another remaining CSE occurrence, iterated to a fixed point so chains of in-place updates keep their memory-operand forms. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Per review, the xarch CSE screen for in-place memory updates is split into its own PR. This PR no longer limits forwarding for read-modify-write sources, so update the X64 UpdateFlags check to expect a single load. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
…137033) Split from #136612. As requested in review, this does the screen in CSE (run normal candidate selection, then post-screen candidates that would become RMW on xarch) and lands it separately with benchmarks/aspnet diffs. ## Change On xarch, CSE can replace (or define) the load in `*p = *p op x`. That keeps the loaded value in a register, so lowering can't emit `op [mem], x`. This PR adds a post-screen to `optValnumCSE_Locate`, `optValnumCSE_ScreenMemoryUpdates` (`TARGET_XARCH` only): - For `STOREIND(addr, op(IND(addr), x))`, the load's CSE occurrence is cleared. `op` must have an RMW memory-operand form (`OperIsRMWMemOp`, or `NOT`/`NEG`). The load must be op1 unless `op` is commutative. Addresses are matched with `GenTree::Compare`, as lowering does. The candidate's remaining occurrences can still be CSE'd. - A load is kept if `op` is itself a CSE candidate with another remaining occurrence, since the result of `op` then has other uses. Removing a load can remove the only other occurrence of another load's `op` (`*p &= a; *p |= b;`), so this is iterated to a fixed point. - `optValnumCSE_InitDataFlow` skips cleared occurrences. Later phases already check `IS_CSE_INDEX`. Example (`InPlaceMemoryUpdate.OrField`, `int old = holder.Flags; holder.Flags |= 0x40; return old;`): ```diff mov eax, dword ptr [rcx+0x10] - mov edx, eax - or eax, 64 - mov dword ptr [rcx+0x10], eax - mov eax, edx + or dword ptr [rcx+0x10], 64 ``` ## Diffs Local SuperPMI asmdiffs, windows x64, Checked JITs: base is main (5c99482), diff is this PR. Current main's JIT-EE GUID has no benchmarks/aspnet collections yet, so these replay the newest available ones with `-jit_ee_version`: - benchmarks: `aec48fc3` (Oct 29). Only 2 and 11 contexts miss. - aspnet2: `fa0c6a6f` (Sep 23, the newest windows x64 aspnet2). 23,709 of 41,203 contexts miss and 567 error, the same for base and diff, so aspnet coverage is only about 41%. No context fails with only one of the two JITs. | Collection | Contexts | Methods with diffs | Smaller / larger | Size | PerfScore (better / worse methods) | |---|--:|--:|--:|--:|--:| | benchmarks.run | 46,242 | 316 | 291 / 24 | -1,795 B (-0.009%) | +0.015% (19 / 294) | | benchmarks.run_pgo | 86,655 | 400 | 395 / 1 | -2,305 B (-0.007%) | +0.018% (25 / 346) | | aspnet2.run | 16,927 | 10 | 10 / 0 | -76 B (-0.004%) | +0.002% (3 / 7) | Code size goes down almost everywhere. PerfScore goes up because it rates the memory-operand form as slower than load/op/store. For example, `inc dword ptr [mem]` scores higher than `inc reg; mov [mem], reg`, even when the value is already in a register. Notable examples: - **Worse, PerfScore only.** `Perf_Regex_Industry_SliceSlice:Count` (run_pgo, Tier1-OSR, -4 B, +16,404 PerfScore; most of the run_pgo total). In a block with weight 10,935, a field is compared and then incremented. With one occurrence left, the field isn't CSE'd any more: ```diff - mov ecx, dword ptr [r13+0x4C] - cmp ecx, eax + cmp dword ptr [r13+0x4C], eax ... - inc ecx - mov dword ptr [r13+0x4C], ecx + inc dword ptr [r13+0x4C] ``` `RegexInterpreter:Scan`, `Regex:RunAllMatchesWithCallback` and `SciMark2.kernel:RandomMatrix` are the same shape in hot loops. - **Worse, PerfScore only.** `System.Diagnostics.StackTrace:ToString` (-60 B, +114 PerfScore) has inlined `StringBuilder.Append(char)`: `inc esi; mov dword ptr [rbx+0x18], esi` becomes `inc dword ptr [rbx+0x18]` in each append. This shape accounts for many of the small diffs. - **Better.** `FrozenHashTable:CalcNumBuckets` (benchmarks.run, -63 B, -56 PerfScore): lower register pressure removes several spills and reloads in the loop and shrinks the frame by 16 bytes. - **Better.** `Benchstone.BenchI.Pi:ComputePi` (-21 B): no `push rdi` / `push rsi`, although PerfScore is +25 because of the RMW form in the loop. For the same change on top of #136612 (with VN store forwarding), libraries.pmi was -17,809 B with PerfScore +0.015%, and libraries.crossgen2 was -2,877 B with PerfScore +0.005%. ## Testing - New disasm test `JIT/opt/CSE/InPlaceMemoryUpdate` covers `or`/`add`/commuted `or` on fields and `and`/`not`/`neg` through a ref. Each method reads the value before the update, so main CSEs the load today. I compiled it with crossgen2 for windows and linux x64 and ran SuperFileCheck: all 6 checks fail with the base JIT and pass with this PR on both OSes. I didn't run the test itself (the managed asserts) locally on x64. - `./build.sh clr.alljits+clr.spmi -c Checked` and `./build.sh clr.alljits -c Release`: no warnings or errors. - `jitformat.py`: clean. > [!NOTE] > This PR description was drafted with GitHub Copilot (AI-generated) and reviewed before posting. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
@AndyAyersMS any thoughts on this one? |
Let's settle #137032 first, and then take another look at this. |
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. --------- Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
Stores through an unknown address (
STOREINDnot to a field, array element or local) are value-numbered as an opaque memory mutation, so a reload from the address just written always gets a fresh VN. This implements theVNF_ByrefExposedStoreidea from the existing comment infgValueNumberStore. A non-volatile int/long/float/double/ref/byref store records a memory state carrying its address and value; long is excluded where it is decomposed. This also applies when the byref-exposed and GC heap memory states differ. A following same-type load from the same liberal address while that state is current takes the stored value.NullPtrExc. VN does not change IR flags.op [mem], x. Screening such loads in CSE is split into a separate PR.Motivation:
Utf8JsonReader.SkipWhiteSpaceon Wasm R2R reloads_consumedafter each store. This removes the loop-latch reload. In an isolated-method d8 measurement, the 64-space case went from 77 to 45 ns.SuperPMI shrinks code on every platform (arm64 −34 KB/−33 KB, arm −17 KB, x64/x86 about −7 KB each, wasm −1 KB). PerfScore improves in every collection, and throughput cost is +0.01–0.07%. The remaining x64/x86 size growth is mostly a read after an update now coming from a register. Details and examples are in the SuperPMI comment; those numbers predate the ref/byref changes and the removal of the xarch read-modify-write check.
Tests: ARM64/X64 disasm checks in
ByrefStoreForwarding, and correctness and exception-preservation cases inExceptionSets. These include a hoisting regression that fails if the loop-dependence record is removed.Note
This PR description and the change were prepared with GitHub Copilot.