JIT: Defer physical promotion readbacks across basic blocks

[jakobbotsch]

Before this PR the model in physical promotion is that promoted locals are always up-to-date in their replacement locals at the end of basic blocks. In certain cases, however, that causes us to be overly eager in insertion of readbacks.

One common case is for struct parameters. Currently the above model means we will always extract fields from the parameters at the end of the entry basic block. If the parameter has many fields in it this leads to creating a lot of live state. Also, if some fields are only used rarely it is wasteful to extract them in the entry block (the extraction may require bitwise operations).

This PR tries to improve the situation. It changes physical promotion to run the replacement phase in RPO. Then, it introduces logic to delay the readbacks:

1. If at the beginning of a basic block all predecessors agree that the local is up to date in the struct, then inherit that in the current basic block and avoid inserting readbacks.
2. If not, insert a reconciling readback at the end of the predecessors where the replacement field is not up to date.

Backedges are handled conservatively by always ensuring replacement fields are up to date into blocks that are targets of backedges.

The above may result in suboptimal insertion of reconciling readbacks in some cases compared to the current behavior. To handle this we additionally have a plan phase where we compute the dominator of all blocks where we expect to insert reconciling readbacks. Then, we use profile information to evaluate whether inserting a common readback in the dominator would be beneficial instead of inserting the multiple reconciling readbacks.

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

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

[jakobbotsch]

cc @dotnet/jit-contrib PTAL @AndyAyersMS

Diffs. Positive perfscore diffs, but generally size regressions. That's expected: inserting the readbacks eagerly only in the entry block is generally smaller code, while delaying them to the actual points where they are used results in multiple readbacks (but overall fewer executed readbacks).

There are some TP regressions. The detailed TP regressions look like this. It looks like a lot of different inlining decisions, but I cannot exclude that the changes to StartBlock/EndBlock are also causing some differences. I have an idea to reduce the work we are doing in those, so I will look into that as a follow up.

This is the "proper" fix for #133603.

[Copilot]

Copilot review overview

🟡 Changes recommended

Add coverage for common readback placement and validate codegen and performance impacts.

Get a fresh assessment by requesting another Copilot review.

Review effort: Lite
Findings: 1

Open (1)

• Add compile-time and codegen measurements for the new heuristic · New

What changed in this PR

This PR defers physical-promotion readbacks across basic blocks using RPO tracking and PGO-guided placement.

Changes:

• Adds deferred readback tracking and reconciliation.
• Adds dominator-based common readback planning.
• Adds branch, loop, EH, and partial-write tests.

File	Description
src/​tests/​JIT/​Directed/​physicalpromotion/​physicalpromotion.cs	Adds physical-promotion regression coverage.
src/​coreclr/​jit/​promotionliveness.cpp	Adds replacement use/definition queries.
src/​coreclr/​jit/​promotion.h	Adds readback tracking and planning state.
src/​coreclr/​jit/​promotion.cpp	Implements deferred readback planning and insertion.

[jakobbotsch]

Like these remarks mention, this is only a hint to the replacement phase after to materialize a readback eagerly even when it wouldn't normally be necessary. Whatever this function computes should not matter for correctness.

[jakobbotsch]

Hmm, I need to double check that this can't place readbacks in blocks that will be skipped when we redirect flow for OSR.

[jakobbotsch]

This diff in System.Guid.CompareTo is representative of a case where this really matters:

 G_M35304_IG01:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, prolog, nogc
-       push     r15
-       push     r14
-       push     rdi
-       push     rsi
-       push     rbp
-       push     rbx
-						;; size=8 bbWeight=1 PerfScore 6.00
+						;; size=0 bbWeight=1 PerfScore 0.00
 G_M35304_IG02:        ; bbWeight=1, gcrefRegs=0000 {}, byrefRegs=0006 {rcx rdx}, byref
        ; byrRegs +[rcx rdx]
        mov      r8d, dword ptr [rdx]
-       movsx    r10, word  ptr [rdx+0x04]
-       movsx    r9, word  ptr [rdx+0x06]
-       movzx    r11, byte  ptr [rdx+0x08]
-       movzx    rbx, byte  ptr [rdx+0x09]
-       movzx    rsi, byte  ptr [rdx+0x0A]
-       movzx    rdi, byte  ptr [rdx+0x0B]
-       movzx    rbp, byte  ptr [rdx+0x0C]
-       movzx    r14, byte  ptr [rdx+0x0D]
-       movzx    r15, byte  ptr [rdx+0x0E]
-       movzx    rdx, byte  ptr [rdx+0x0F]
-       ; byrRegs -[rdx]
-       cmp      r8d, dword ptr [rcx]
+       mov      r10d, dword ptr [rcx]
+       cmp      r8d, r10d
        jne      G_M35304_IG15
-						;; size=60 bbWeight=1 PerfScore 25.00
-G_M35304_IG03:        ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0002 {rcx}, byref, isz
-       movsx    r8, word  ptr [rcx+0x04]
-       cmp      r10d, r8d
+						;; size=15 bbWeight=1 PerfScore 5.25
+G_M35304_IG03:        ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0006 {rcx rdx}, byref, isz
+       movsx    r8, word  ptr [rdx+0x04]
+       movsx    r10, word  ptr [rcx+0x04]
+       cmp      r8d, r10d
        jne      G_M35304_IG14
-       movsx    r8, word  ptr [rcx+0x06]
-       cmp      r9d, r8d
+       movsx    r8, word  ptr [rdx+0x06]
+       movsx    r10, word  ptr [rcx+0x06]
+       cmp      r8d, r10d
        jne      G_M35304_IG13
-       movzx    r8, byte  ptr [rcx+0x08]
-       cmp      r11d, r8d
+       movzx    r8, byte  ptr [rdx+0x08]
+       movzx    r10, byte  ptr [rcx+0x08]
+       cmp      r8d, r10d
        jne      G_M35304_IG12
-       movzx    r8, byte  ptr [rcx+0x09]
-       cmp      ebx, r8d
+       movzx    r8, byte  ptr [rdx+0x09]
+       movzx    r10, byte  ptr [rcx+0x09]
+       cmp      r8d, r10d
        jne      G_M35304_IG11
-       movzx    r8, byte  ptr [rcx+0x0A]
-       cmp      esi, r8d
+       movzx    r8, byte  ptr [rdx+0x0A]
+       movzx    r10, byte  ptr [rcx+0x0A]
+       cmp      r8d, r10d
        jne      G_M35304_IG10
-       movzx    r8, byte  ptr [rcx+0x0B]
-       cmp      edi, r8d
+       movzx    r8, byte  ptr [rdx+0x0B]
+       movzx    r10, byte  ptr [rcx+0x0B]
+       cmp      r8d, r10d
        jne      G_M35304_IG09
-       movzx    r8, byte  ptr [rcx+0x0C]
-       cmp      ebp, r8d
+       movzx    r8, byte  ptr [rdx+0x0C]
+       movzx    r10, byte  ptr [rcx+0x0C]
+       cmp      r8d, r10d
        jne      SHORT G_M35304_IG08
-       movzx    r8, byte  ptr [rcx+0x0D]
-       cmp      r14d, r8d
+       movzx    r8, byte  ptr [rdx+0x0D]
+       movzx    r10, byte  ptr [rcx+0x0D]
+       cmp      r8d, r10d
        jne      SHORT G_M35304_IG07
-       movzx    r8, byte  ptr [rcx+0x0E]
-       cmp      r15d, r8d
+       movzx    r8, byte  ptr [rdx+0x0E]
+       movzx    r10, byte  ptr [rcx+0x0E]
+       cmp      r8d, r10d
        jne      SHORT G_M35304_IG06
-       movzx    r8, byte  ptr [rcx+0x0F]
-       cmp      edx, r8d
+       movzx    r8, byte  ptr [rdx+0x0F]
+       movzx    r10, byte  ptr [rcx+0x0F]
+       cmp      r8d, r10d
        jne      SHORT G_M35304_IG05
        xor      eax, eax
-						;; size=126 bbWeight=0.50 PerfScore 16.38
+						;; size=176 bbWeight=0.50 PerfScore 26.38
 G_M35304_IG04:        ; bbWeight=0.50, epilog, nogc, extend
-       pop      rbx
-       pop      rbp
-       pop      rsi
-       pop      rdi
-       pop      r14
-       pop      r15
        ret      
-						;; size=9 bbWeight=0.50 PerfScore 2.00
+						;; size=1 bbWeight=0.50 PerfScore 0.50
 G_M35304_IG05:        ; bbWeight=0.50, gcVars=0000000000000000 {}, gcrefRegs=0000 {}, byrefRegs=0000 {}, gcvars, byref
-       ; byrRegs -[rcx]
+       ; byrRegs -[rcx rdx]
        mov      eax, -1
-       mov      r11d, 1
-       cmp      r8d, edx
-       cmovge   eax, r11d
+       mov      ecx, 1
+       cmp      r10d, r8d
+       cmovge   eax, ecx
        jmp      G_M35304_IG16
-						;; size=23 bbWeight=0.50 PerfScore 1.62
+						;; size=21 bbWeight=0.50 PerfScore 1.62
 G_M35304_IG06:        ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
        mov      eax, -1
-       mov      r9d, 1
-       cmp      r8d, r15d
-       cmovge   eax, r9d
+       mov      ecx, 1
+       cmp      r10d, r8d
+       cmovge   eax, ecx
        jmp      G_M35304_IG16
-						;; size=23 bbWeight=0.50 PerfScore 1.62
+						;; size=21 bbWeight=0.50 PerfScore 1.62
 G_M35304_IG07:        ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref
        mov      eax, -1
-       mov      r10d, 1
-       cmp      r8d, r14d
-       cmovge   eax, r10d
+       mov      ecx, 1
+       cmp      r10d, r8d
+       cmovge   eax, ecx
        jmp      G_M35304_IG16
-						;; size=23 bbWeight=0.50 PerfScore 1.62
+						;; size=21 bbWeight=0.50 PerfScore 1.62
 G_M35304_IG08:        ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
        mov      eax, -1
        mov      ecx, 1
-       cmp      r8d, ebp
+       cmp      r10d, r8d
        cmovge   eax, ecx
        jmp      SHORT G_M35304_IG16
 						;; size=18 bbWeight=0.50 PerfScore 1.62
 G_M35304_IG09:        ; bbWeight=0.50, gcrefRegs=0000 {}, byrefRegs=0000 {}, byref, isz
        mov      eax, -1
        mov      ecx, 1
-       cmp      r8d, edi
-

(There might be some missing forward sub, need to take a closer look at why we don't get containment here)