Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

JIT: CSE of an address computation can result in a write barrier change #97534

Closed
AndyAyersMS opened this issue Jan 26, 2024 · 4 comments · Fixed by #97953
Closed

JIT: CSE of an address computation can result in a write barrier change #97534

AndyAyersMS opened this issue Jan 26, 2024 · 4 comments · Fixed by #97953
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@AndyAyersMS
Copy link
Member

AndyAyersMS commented Jan 26, 2024

If we CSE an address computation for a GC store, then gcIsWriteBarrierCandidate may no longer be able to tell that the address being stored to is a heap address, and so will switch to using a checked write barrier.

Seems like we could find a way not to lose track of this, info for example marking the store as GTF_IND_TGT_HEAP (or GTF_IND_TGT_NOT_HEAP) if we do a CSE.

For example:

;; compiling System.Int32:CompareTo(int):int:this (MethodHash=c9004add)

CSE #05 def at [000177] replaced in BB06 with def of V12
BB06 requires throw helper block for SCK_RNGCHK_FAIL
optValnumCSE morphed tree:
N023 ( 23, 27)              [000086] DA-XGO-----                         *  STORE_LCL_VAR int    V11 tmp10        d:1 <l:$199, c:$19d>
N022 ( 19, 24)              [000085] -A-XGO-----                         \--*  SUB       int    <l:$295, c:$294>
N020 ( 17, 22)              [000081] VA-XGO-----                            +--*  IND       int    <l:$292, c:$291>
N019 ( 15, 20)              [000165] -A-XGO-N---                            |  \--*  ADD       byref  <l:$20e, c:$20f>
N017 ( 14, 19)              [000232] -A-X-O-----                            |     +--*  COMMA     byref  <l:$20d, c:$20c>
N015 ( 13, 18) CSE #05 (def)[000230] DA-X-O-----                            |     |  +--*  STORE_LCL_VAR byref  V12 cse0         d:1 $VN.Void
N014 ( 13, 18)              [000177] ---X-O-----                            |     |  |  \--*  COMMA     byref  <l:$20d, c:$20c>
N004 (  8, 11)              [000169] ---X-O-----                            |     |  |     +--*  BOUNDS_CHECK_Rng void   <l:$199, c:$198>
N001 (  1,  1)              [000078] -----------                            |     |  |     |  +--*  LCL_VAR   int    V09 tmp8         u:1 <l:$28b, c:$28c>
N003 (  3,  3) CSE #02 (def)[000168] ---X-------                            |     |  |     |  \--*  ARR_LENGTH int    <l:$28f, c:$290>
N002 (  1,  1)              [000077] -----------                            |     |  |     |     \--*  LCL_VAR   ref    V07 tmp6         u:1 <l:$189, c:$108>
N013 (  5,  7)              [000176] -----O-----                            |     |  |     \--*  ARR_ADDR  byref System.Collections.Concurrent.ConcurrentQueueSegment`1+Slot[] $81
N012 (  5,  7)              [000175] -------N---                            |     |  |        \--*  ADD       byref  <l:$20a, c:$20b>
N005 (  1,  1)              [000166] -----------                            |     |  |           +--*  LCL_VAR   ref    V07 tmp6         u:1 <l:$189, c:$108>
N011 (  5,  6)              [000174] -------N---                            |     |  |           \--*  ADD       long   <l:$3c4, c:$3c5>
N009 (  4,  5) CSE #04 (def)[000172] -----------                            |     |  |              +--*  LSH       long   <l:$3c2, c:$3c3>
N007 (  2,  3) CSE #03 (def)[000170] ---------U-                            |     |  |              |  +--*  CAST      long <- uint <l:$3c0, c:$3c1>
N006 (  1,  1)              [000167] -----------                            |     |  |              |  |  \--*  LCL_VAR   int    V09 tmp8         u:1 <l:$28b, c:$28c>
N008 (  1,  1)              [000171] -------N---                            |     |  |              |  \--*  CNS_INT   long   4 $1c6
N010 (  1,  1)              [000173] -----------                            |     |  |              \--*  CNS_INT   long   16 $1c0
N016 (  1,  1)              [000231] -----------                            |     |  \--*  LCL_VAR   byref  V12 cse0         u:1 <l:$20d, c:$20c>
N018 (  1,  1)              [000164] -----------                            |     \--*  CNS_INT   long   8 $1c3
N021 (  1,  1)              [000084] -----------                            \--*  LCL_VAR   int    V08 tmp7         u:1 $241

-----
Generating: N191 (  1,  1) [000233] -----------                  t233 =    LCL_VAR   byref  V12 cse0         u:1 r14 REG r14 $81
Generating: N193 (  1,  1) [000009] -----------                    t9 =    LCL_VAR   ref    V00 this         u:1 rbx (last use) REG rbx $100
                                                                        /--*  t233   byref  
                                                                        +--*  t9     ref    
Generating: N195 (  5,  4) [000114] nA--GO-----                         *  STOREIND  ref    REG NA <l:$199, c:$198>
							V00 in reg rbx is becoming dead  [000009]
							Live regs: 000000000000C048 {rbx rsi r14 r15} - {rbx} => 000000000000C040 {rsi r14 r15}
Debug: Closing V00 debug range.
							Live vars after [000009]: {V00 V02 V12 V13} -{V00} => {V02 V12 V13}
							GC regs: 0048 {rbx rsi} => 0040 {rsi}
Mapped BB08 to G_M63573_IG08
IN0029:        mov      rcx, r14
IN002a:        mov      rdx, rbx
NoGC Call: savedSet=F0E8 {rbx rbp rsi rdi r12 r13 r14 r15}
							Call: GCvars=0000000000000000 {}, gcrefRegs=0040 {rsi}, byrefRegs=4000 {r14}
IN002b:        call     CORINFO_HELP_CHECKED_ASSIGN_REF

We may also want to propagate this info during assertion prop or similar so if there is a manual CSE we still are able to see through it.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jan 26, 2024
@ghost ghost added the untriaged New issue has not been triaged by the area owner label Jan 26, 2024
@ghost
Copy link

ghost commented Jan 26, 2024

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

Issue Details

If we CSE an address computation for a GC store, then gcIsWriteBarrierCandidate may no longer be able to tell that the address is a heap address, and so will switch to using a checked write barrier.

Seems like we could find a way not to lose track of this, info for example marking the store as GTF_IND_TGT_HEAP (or GTF_IND_TGT_NOT_HEAP) if we do a CSE.

For example:

;; compiling System.Int32:CompareTo(int):int:this (MethodHash=c9004add)

CSE #05 def at [000177] replaced in BB06 with def of V12
BB06 requires throw helper block for SCK_RNGCHK_FAIL
optValnumCSE morphed tree:
N023 ( 23, 27)              [000086] DA-XGO-----                         *  STORE_LCL_VAR int    V11 tmp10        d:1 <l:$199, c:$19d>
N022 ( 19, 24)              [000085] -A-XGO-----                         \--*  SUB       int    <l:$295, c:$294>
N020 ( 17, 22)              [000081] VA-XGO-----                            +--*  IND       int    <l:$292, c:$291>
N019 ( 15, 20)              [000165] -A-XGO-N---                            |  \--*  ADD       byref  <l:$20e, c:$20f>
N017 ( 14, 19)              [000232] -A-X-O-----                            |     +--*  COMMA     byref  <l:$20d, c:$20c>
N015 ( 13, 18) CSE #05 (def)[000230] DA-X-O-----                            |     |  +--*  STORE_LCL_VAR byref  V12 cse0         d:1 $VN.Void
N014 ( 13, 18)              [000177] ---X-O-----                            |     |  |  \--*  COMMA     byref  <l:$20d, c:$20c>
N004 (  8, 11)              [000169] ---X-O-----                            |     |  |     +--*  BOUNDS_CHECK_Rng void   <l:$199, c:$198>
N001 (  1,  1)              [000078] -----------                            |     |  |     |  +--*  LCL_VAR   int    V09 tmp8         u:1 <l:$28b, c:$28c>
N003 (  3,  3) CSE #02 (def)[000168] ---X-------                            |     |  |     |  \--*  ARR_LENGTH int    <l:$28f, c:$290>
N002 (  1,  1)              [000077] -----------                            |     |  |     |     \--*  LCL_VAR   ref    V07 tmp6         u:1 <l:$189, c:$108>
N013 (  5,  7)              [000176] -----O-----                            |     |  |     \--*  ARR_ADDR  byref System.Collections.Concurrent.ConcurrentQueueSegment`1+Slot[] $81
N012 (  5,  7)              [000175] -------N---                            |     |  |        \--*  ADD       byref  <l:$20a, c:$20b>
N005 (  1,  1)              [000166] -----------                            |     |  |           +--*  LCL_VAR   ref    V07 tmp6         u:1 <l:$189, c:$108>
N011 (  5,  6)              [000174] -------N---                            |     |  |           \--*  ADD       long   <l:$3c4, c:$3c5>
N009 (  4,  5) CSE #04 (def)[000172] -----------                            |     |  |              +--*  LSH       long   <l:$3c2, c:$3c3>
N007 (  2,  3) CSE #03 (def)[000170] ---------U-                            |     |  |              |  +--*  CAST      long <- uint <l:$3c0, c:$3c1>
N006 (  1,  1)              [000167] -----------                            |     |  |              |  |  \--*  LCL_VAR   int    V09 tmp8         u:1 <l:$28b, c:$28c>
N008 (  1,  1)              [000171] -------N---                            |     |  |              |  \--*  CNS_INT   long   4 $1c6
N010 (  1,  1)              [000173] -----------                            |     |  |              \--*  CNS_INT   long   16 $1c0
N016 (  1,  1)              [000231] -----------                            |     |  \--*  LCL_VAR   byref  V12 cse0         u:1 <l:$20d, c:$20c>
N018 (  1,  1)              [000164] -----------                            |     \--*  CNS_INT   long   8 $1c3
N021 (  1,  1)              [000084] -----------                            \--*  LCL_VAR   int    V08 tmp7         u:1 $241

-----
Generating: N191 (  1,  1) [000233] -----------                  t233 =    LCL_VAR   byref  V12 cse0         u:1 r14 REG r14 $81
Generating: N193 (  1,  1) [000009] -----------                    t9 =    LCL_VAR   ref    V00 this         u:1 rbx (last use) REG rbx $100
                                                                        /--*  t233   byref  
                                                                        +--*  t9     ref    
Generating: N195 (  5,  4) [000114] nA--GO-----                         *  STOREIND  ref    REG NA <l:$199, c:$198>
							V00 in reg rbx is becoming dead  [000009]
							Live regs: 000000000000C048 {rbx rsi r14 r15} - {rbx} => 000000000000C040 {rsi r14 r15}
Debug: Closing V00 debug range.
							Live vars after [000009]: {V00 V02 V12 V13} -{V00} => {V02 V12 V13}
							GC regs: 0048 {rbx rsi} => 0040 {rsi}
Mapped BB08 to G_M63573_IG08
IN0029:        mov      rcx, r14
IN002a:        mov      rdx, rbx
NoGC Call: savedSet=F0E8 {rbx rbp rsi rdi r12 r13 r14 r15}
							Call: GCvars=0000000000000000 {}, gcrefRegs=0040 {rsi}, byrefRegs=4000 {r14}
IN002b:        call     CORINFO_HELP_CHECKED_ASSIGN_REF

We may also want to propagate this info during assertion prop or similar so if there is a manual CSE we still are able to see through it.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS AndyAyersMS removed the untriaged New issue has not been triaged by the area owner label Jan 26, 2024
@AndyAyersMS AndyAyersMS added this to the Future milestone Jan 26, 2024
AndyAyersMS added a commit to AndyAyersMS/runtime that referenced this issue Jan 26, 2024
When varying CSEs we can sometimes alter the write barrier that is
needed. In particular if we CSE a heap address computation we may
lose track of the fact that an indir is writing to the heap, and so
change which write barrier needs to be used.

See dotnet#97534.

Even if that's fixed it seems like we still might change our minds
for various reasons, so when running under SPMI, just collect all
the possible write barrier helper addresses.
AndyAyersMS added a commit that referenced this issue Jan 26, 2024
When varying CSEs we can sometimes alter the write barrier that is
needed. In particular if we CSE a heap address computation we may
lose track of the fact that an indir is writing to the heap, and so
change which write barrier needs to be used.

See #97534.

Even if that's fixed it seems like we still might change our minds
for various reasons, so when running under SPMI, just collect all
the possible write barrier helper addresses.
@EgorBo
Copy link
Member

EgorBo commented Jan 26, 2024

Minimal repro:

void Test(ref string s1, ref string s2)
{
    s1 = "Hello";
    s2 = "Hello";
}

Codegen:

; Method Program:Test(byref,byref):this (FullOpts)
       push     rsi
       push     rbx
       mov      rcx, rdx
       mov      rbx, r8
       mov      rsi, 0x22080106290
       mov      rdx, rsi
       call     CORINFO_HELP_CHECKED_ASSIGN_REF
       mov      rcx, rbx
       mov      rdx, rsi
       call     CORINFO_HELP_CHECKED_ASSIGN_REF
       nop      
       pop      rbx
       pop      rsi
       ret      
; Total bytes of code: 41

Now similar code but different string literals:

void Test(ref string s1, ref string s2)
{
    s1 = "Hello";
    s2 = "World";
}

Codegen:

; Method Program:Test(byref,byref):this (FullOpts)
       mov      rax, 0x24F00106290
       mov      gword ptr [rdx], rax
       mov      rax, 0x24F001062B0
       mov      gword ptr [r8], rax
       ret      
; Total bytes of code: 27

(frozen objects need no write barrier, but CSE might mess it up)

@AndyAyersMS
Copy link
Member Author

@EgorBo that's actually a slightly different (but related) problem.

Write barrier form depends both on what is being written (your example) and where it is being written to (my example). CSE can mess with either or both.

Interestingly somebody (haven't looked at blame yet to see who) added code to use VN to examine what is being written. Seems a bit iffy to look at VNs so late, but if it is viable, one could imagine doing something comparable for where it's being written (I suspect addr mode formation will get in the way, and we'll have to walk the IR like we do now, and fall down if we hit a cse).

I'd rather start depending on the TGT flags being accurate, for the where part of all this.

@EgorBo
Copy link
Member

EgorBo commented Feb 4, 2024

Interestingly somebody (haven't looked at blame yet to see who) added code to use VN to examine what is being written.

It wasn't me, but I filed #97943 to remove it 🙂

@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 5, 2024
@EgorBo EgorBo modified the milestones: Future, 9.0.0 Feb 5, 2024
@EgorBo EgorBo self-assigned this Feb 5, 2024
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 8, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Mar 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants