[lewing]

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;):

        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:

  -       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.

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

I took a look at this and the other PR because I'm working on CSE at the moment in #136950

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:

That argument doesn't bear out, doing store to load forwarding here will cause it to cost more cycles in loops (and similar cases) due to stalls. Same for the other examples. It really is worse and only saves on size.

[lewing]

Thanks for looking. I agree that "PerfScore only" was too generous for the loop cases. Here is the hot block in Perf_Regex_Industry_SliceSlice:Count (weight ~10.9k):

base                                  diff
mov  ecx, dword ptr [r13+0x4C]        cmp  dword ptr [r13+0x4C], eax
cmp  ecx, eax                         ...
...                                   inc  dword ptr [r13+0x4C]
inc  ecx
mov  dword ptr [r13+0x4C], ecx

The field isn't loop-invariant, so both versions reload it every iteration, and the loop-carried store-to-load forward is the same in both. Nothing new stalls here. The real cost is one extra load per iteration: cmp and inc each read memory, where base shares a single load. So it's a small real regression, not just a PerfScore artifact.

The screen wins when the other occurrence needs the value in a register anyway (old = x; x |= c; return old; drops two movs). It loses when the other occurrence could have contained the load itself, like cmp [mem], reg. I can tighten it to keep the CSE when every remaining occurrence is containable. Or I can close this if you'd rather handle it as part of #136950.

Note

This reply was drafted with GitHub Copilot (AI-generated).

[NinoFloris]

The field isn't loop-invariant

Missed that, then yeah it's a small penalty only.

Or I can close this if you'd rather handle it as part of #136950.

Keep it separate. I was mostly curious about the effects of the other PR because it affects how many loads can get CSE'd (also I have far too many PRs already that will have to be reviewed).

[tannergooding]

This probably deserves weigh-in from @dotnet/intel and how it may impact some APX work.

[DeepakRajendrakumaran]

This probably deserves weigh-in from @dotnet/intel and how it may impact some APX work.

From the APX side, I see two effects. First, the screen frees a register. So, it shrinks the opportunities where eGPRs would have helped, though the resulting code is still smaller on both sides. Second, NDD would apply to the shared load/op/store form, but the PR's version is still preferable on size and registers, at the cost of one extra load:

 // Current                                 // With NDD                          //   With this PR

 mov  r1, [mem]                            mov  r1, [mem]                          mov  r1, [mem] 
 mov  r3, r1                               add  r3, r1, r2                         add  [mem], r2
 add  r3, r2                                mov  [mem], r3
 mov  [mem], r3

Can you think of any other APX impact we might have missed?