Stack allocate unescaped boxes #103361

AndyAyersMS · 2024-06-12T18:15:05Z

Enable object stack allocation for ref classes and extend the support to include boxed value classes. Use a specialized unbox helper for stack allocated boxes, both to avoid apparent escape of the box by the helper, and to ensure all box field accesses are visible to the JIT. Update the local address visitor to rewrite trees involving address of stack allocated boxes in some cases to avoid address exposure. Disable old promotion for stack allocated boxes (since we have no field handles) and allow physical promotion to enregister the box method table and/or payload as appropriate. In OSR methods handle the fact that the stack allocation may actually have been a heap allocation by the Tier0 method.

The analysis TP cost is around 0.4-0.7% (notes below). Boxes are much less likely to escape than ref classes (roughly ~90% of boxes escape, ~99.8% of ref classes escape). Codegen impact is diminished somewhat because many of the boxes are dead and were already getting optimized away.

Fixes #4584, #9118, #10195, #11192, #53585, #58554, #85570

Still todo:

settle on the right new type for a stack allocated box (currently KeyValuePair)
R2R/NAOT support
continue trying to do a realistic assessment of impact

…-lclmorph Minor fixes

AndyAyersMS · 2024-06-12T18:30:49Z

For case from #9118:

; Assembly listing for method C:Main():int (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 1 inlinees with PGO data; 3 single block inlinees; 1 inlinees without PGO data
; Final local variable assignments
;
;  V00 OutArgs      [V00    ] (  1,  1   )  struct (32) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V01 tmp1         [V01,T00] (  0,  0   )     ref  ->  zero-ref    class-hnd exact single-def "NewObj constructor temp" <ObjectEqualityComparer`1[int]>
;* V02 tmp2         [V02    ] (  0,  0   )   ubyte  ->  zero-ref    "Inline return value spill temp"
;* V03 tmp3         [V03    ] (  0,  0   )     int  ->  zero-ref    ld-addr-op "Inlining Arg"
;* V04 tmp4         [V04    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <System.Int32>
;* V05 tmp5         [V05    ] (  0,  0   )   ubyte  ->  zero-ref    "Inline return value spill temp"
;* V06 tmp6         [V06    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Inlining Arg" <System.Int32>
;* V07 tmp7         [V07    ] (  0,  0   )  struct (16) zero-ref    do-not-enreg[SF] "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,int]>
;* V08 tmp8         [V08    ] (  0,  0   )    long  ->  zero-ref    "V07.[000..008)"
;* V09 tmp9         [V09    ] (  0,  0   )     int  ->  zero-ref    "V07.[008..012)"
;
; Lcl frame size = 40

G_M46144_IG01:  ;; offset=0x0000
       sub      rsp, 40
                                                ;; size=4 bbWeight=1 PerfScore 0.25
G_M46144_IG02:  ;; offset=0x0004
       mov      eax, 100
                                                ;; size=5 bbWeight=1 PerfScore 0.25
G_M46144_IG03:  ;; offset=0x0009
       add      rsp, 40
       ret
                                                ;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 14, prolog size 4, PerfScore 1.75, instruction count 4, allocated bytes for code 14 (MethodHash=cd934bbf) for method C:Main():int (FullOpts)

AndyAyersMS · 2024-06-12T18:32:51Z

Current code for #10195

; Assembly listing for method Program:Pattern[Dog](Dog) (FullOpts)
; Emitting BLENDED_CODE for X86 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  3,  3   )  struct ( 8) rcx         single-def <Dog>
;* V01 loc0         [V01    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact <Dog>
;# V02 OutArgs      [V02    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V03 tmp1         [V03    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <Dog>
;* V04 tmp2         [V04    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;  V05 tmp3         [V05    ] (  2,  2   )  struct (16) [rsp+0x08]  do-not-enreg[XSF] must-init addr-exposed "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,Dog]>
;
; Lcl frame size = 24

G_M46299_IG01:  ;; offset=0x0000
       sub      rsp, 24
       xor      eax, eax
       mov      qword ptr [rsp+0x08], rax
       mov      qword ptr [rsp+0x10], rax
						;; size=16 bbWeight=1 PerfScore 2.50
G_M46299_IG02:  ;; offset=0x0010
       mov      rax, 0x7FFEDABE4C08      ; Dog
       mov      qword ptr [rsp+0x08], rax
       mov      byte  ptr [rsp+0x10], cl
						;; size=19 bbWeight=1 PerfScore 2.25
G_M46299_IG03:  ;; offset=0x0023
       add      rsp, 24
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 40, prolog size 16, PerfScore 6.00, instruction count 9, allocated bytes for code 40 (MethodHash=a9204b24) for method Program:Pattern[Dog](Dog) (FullOpts)
; ============================================================

No more boxing, but room for improvement...

jakobbotsch · 2024-06-12T18:45:50Z

Current code for #10195

; Assembly listing for method Program:Pattern[Dog](Dog) (FullOpts)
; Emitting BLENDED_CODE for X86 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 arg0         [V00,T00] (  3,  3   )  struct ( 8) rcx         single-def <Dog>
;* V01 loc0         [V01    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact <Dog>
;# V02 OutArgs      [V02    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V03 tmp1         [V03    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <Dog>
;* V04 tmp2         [V04    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;  V05 tmp3         [V05    ] (  2,  2   )  struct (16) [rsp+0x08]  do-not-enreg[XSF] must-init addr-exposed "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,Dog]>
;
; Lcl frame size = 24

G_M46299_IG01:  ;; offset=0x0000
       sub      rsp, 24
       xor      eax, eax
       mov      qword ptr [rsp+0x08], rax
       mov      qword ptr [rsp+0x10], rax
						;; size=16 bbWeight=1 PerfScore 2.50
G_M46299_IG02:  ;; offset=0x0010
       mov      rax, 0x7FFEDABE4C08      ; Dog
       mov      qword ptr [rsp+0x08], rax
       mov      byte  ptr [rsp+0x10], cl
						;; size=19 bbWeight=1 PerfScore 2.25
G_M46299_IG03:  ;; offset=0x0023
       add      rsp, 24
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 40, prolog size 16, PerfScore 6.00, instruction count 9, allocated bytes for code 40 (MethodHash=a9204b24) for method Program:Pattern[Dog](Dog) (FullOpts)
; ============================================================

No more boxing, but room for improvement...

Any idea why we end up with address exposure for V05?

AndyAyersMS · 2024-06-12T18:49:59Z

Any idea why we end up with address exposure for V05?

I think it's that local morph needs to deal with a nullcheck... will know shortly

jkotas · 2024-06-12T18:57:37Z

src/coreclr/vm/jitinterface.cpp

+        TypeHandle mt(CoreLibBinder::GetElementType(ELEMENT_TYPE_I));
+        TypeHandle boxedFields[2] = {mt, VMClsHnd};
+        Instantiation boxedFieldsInst((TypeHandle*)&boxedFields, 2);
+        TypeHandle genericKvp = CoreLibBinder::GetClass(CLASS__KEYVALUEPAIRGENERIC);


This won't work well. KeyValuePair is auto-layout when the type contains references. auto-layout moves references to the front.

For example, KeyValuePair<IntPtr, string> has string at offset 0 and IntPtr at offset 8.

I guess it probably works ok for valuetypes currently, but it is fragile.

Thanks... we were trying to find a suitable type without needing too much from the runtime.

I think we should introduce dedicated internal helper type for this.

We can also try and do this entirely within the jit (that's where I started), but it would be nice to have class and field handles and the like.

I also am still not sure how effective any of this will be, so I want to do some more analysis. Earlier studies suggest we can prove maybe 1% of boxes do not escape (static, not dynamic) but were based on SPMI which can't replay a bunch of possibly interesting cases. Given the cost of the analysis I would like to see if we can get that number a bit higher.

That 1% is a static measure, that is, 1% of the box call sites, not necessarily 1% of boxes allocated.

AndyAyersMS · 2024-06-12T19:21:21Z

Yep, that was the reason...

; Assembly listing for method Program:Pattern[Dog](Dog) (FullOpts)
; Emitting BLENDED_CODE for X86 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;* V00 arg0         [V00    ] (  0,  0   )  struct ( 8) zero-ref    single-def <Dog>
;* V01 loc0         [V01    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact <Dog>
;# V02 OutArgs      [V02    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V03 tmp1         [V03    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <Dog>
;* V04 tmp2         [V04    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;* V05 tmp3         [V05    ] (  0,  0   )  struct (16) zero-ref    do-not-enreg[SF] "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,Dog]>
;
; Lcl frame size = 0

G_M46299_IG01:  ;; offset=0x0000
						;; size=0 bbWeight=1 PerfScore 0.00
G_M46299_IG02:  ;; offset=0x0000
       ret      
						;; size=1 bbWeight=1 PerfScore 1.00

; Total bytes of code 1, prolog size 0, PerfScore 1.00, instruction count 1, allocated bytes for code 1 (MethodHash=a9204b24) for method Program:Pattern[Dog](Dog) (FullOpts)
; ============================================================

AndyAyersMS · 2024-06-12T19:50:28Z

For #85570 one box gets optimized, but the other box is passed to an unbox helper. This helper causes the box to escape if the type tests fail.

There is some preliminary helper escape logic in #103148 but it is too aggressive for unboxing. We would need some kind of partial escape analysis and a different helper pattern for this.

AndyAyersMS · 2024-06-12T20:29:21Z

We stack allocate in #58554 but the local box is exposed, possibly because of a ?: -- need to review if we get conservative handling these in local morph.

So asm is not that pretty yet

; Assembly listing for method Program:Test[System.ValueTuple`2[int,System.__Canon],System.ValueTuple`2[int,System.__Canon]](System.ValueTuple`2[int,System.__Canon]):ubyte (FullOpts)
; Emitting BLENDED_CODE for X86 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; Final local variable assignments
;
;  V00 TypeCtx      [V00,T01] (  4,  4   )    long  ->  rcx         single-def
;  V01 arg0         [V01,T00] (  3,  6   )   byref  ->  rdx         single-def
;# V02 OutArgs      [V02    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V03 tmp1         [V03    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <System.ValueTuple`2[int,System.__Canon]>
;* V04 tmp2         [V04    ] (  0,  0   )   byref  ->  zero-ref    "ISINST eval op1"
;  V05 tmp3         [V05,T02] (  2,  4   )   byref  ->  rdx         class-hnd "spilling qmarkNull" <System.ValueTuple`2[int,System.__Canon]>
;  V06 tmp4         [V06    ] (  4,  4   )  struct (24) [rsp+0x08]  do-not-enreg[XSF] must-init addr-exposed "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,System.ValueTuple`2[int,System.__Canon]]>
;* V07 tmp5         [V07    ] (  0,  0   )     ref  ->  zero-ref    "field V01.Item2 (fldOffset=0x0)" P-INDEP
;* V08 tmp6         [V08    ] (  0,  0   )     int  ->  zero-ref    "field V01.Item1 (fldOffset=0x8)" P-INDEP
;* V09 tmp7         [V09    ] (  0,  0   )  struct (16) zero-ref    "Promoted implicit byref" <System.ValueTuple`2[int,System.__Canon]>
;  V10 cse0         [V10,T03] (  3,  3   )    long  ->  rax         "CSE #01: aggressive"
;
; Lcl frame size = 40

G_M33081_IG01:  ;; offset=0x0000
       sub      rsp, 40
       vxorps   xmm4, xmm4, xmm4
       vmovdqu  xmmword ptr [rsp+0x08], xmm4
       xor      eax, eax
       mov      qword ptr [rsp+0x18], rax
       mov      qword ptr [rsp+0x20], rcx
						;; size=26 bbWeight=1 PerfScore 4.83
G_M33081_IG02:  ;; offset=0x001A
       mov      rax, qword ptr [rcx+0x40]
       mov      rcx, qword ptr [rax]
       mov      qword ptr [rsp+0x08], rcx
						;; size=12 bbWeight=1 PerfScore 5.00
G_M33081_IG03:  ;; offset=0x0026
       vmovdqu  xmm0, xmmword ptr [rdx]
       vmovdqu  xmmword ptr [rsp+0x10], xmm0
						;; size=10 bbWeight=1 PerfScore 5.00
G_M33081_IG04:  ;; offset=0x0030
       mov      rcx, qword ptr [rsp+0x08]
       lea      rdx, bword ptr [rsp+0x08]
       xor      r8, r8
       cmp      rcx, qword ptr [rax+0x08]
       cmovne   rdx, r8
       test     rdx, rdx
       setne    al
       movzx    rax, al
						;; size=30 bbWeight=1 PerfScore 6.50
G_M33081_IG05:  ;; offset=0x004E
       add      rsp, 40
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 83, prolog size 26, PerfScore 22.58, instruction count 21, allocated bytes for code 83 (MethodHash=0b7f7ec6) for method Program:Test[System.ValueTuple`2[int,System.__Canon],System.ValueTuple`2[int,System.__Canon]](System.ValueTuple`2[int,System.__Canon]):ubyte (FullOpts)
; ============================================================

AndyAyersMS · 2024-06-12T20:30:03Z

Will run mihu bot once we're a bit further along in testing, to get a sense of diffs.

jakobbotsch · 2024-06-12T22:18:13Z

src/coreclr/jit/lclmorph.cpp

+                }
+                INDEBUG(TopValue(0).Consume());
+                PopValue();


This has to call EscapeValue in the non-transformed case (that might be the reason for the asserts)

jakobbotsch · 2024-06-13T02:11:20Z

We stack allocate in #58554 but the local box is exposed, possibly because of a ?: -- need to review if we get conservative handling these in local morph.

So asm is not that pretty yet

; Assembly listing for method Program:Test[System.ValueTuple`2[int,System.__Canon],System.ValueTuple`2[int,System.__Canon]](System.ValueTuple`2[int,System.__Canon]):ubyte (FullOpts)
; Emitting BLENDED_CODE for X86 with AVX512 - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; Final local variable assignments
;
;  V00 TypeCtx      [V00,T01] (  4,  4   )    long  ->  rcx         single-def
;  V01 arg0         [V01,T00] (  3,  6   )   byref  ->  rdx         single-def
;# V02 OutArgs      [V02    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V03 tmp1         [V03    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <System.ValueTuple`2[int,System.__Canon]>
;* V04 tmp2         [V04    ] (  0,  0   )   byref  ->  zero-ref    "ISINST eval op1"
;  V05 tmp3         [V05,T02] (  2,  4   )   byref  ->  rdx         class-hnd "spilling qmarkNull" <System.ValueTuple`2[int,System.__Canon]>
;  V06 tmp4         [V06    ] (  4,  4   )  struct (24) [rsp+0x08]  do-not-enreg[XSF] must-init addr-exposed "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,System.ValueTuple`2[int,System.__Canon]]>
;* V07 tmp5         [V07    ] (  0,  0   )     ref  ->  zero-ref    "field V01.Item2 (fldOffset=0x0)" P-INDEP
;* V08 tmp6         [V08    ] (  0,  0   )     int  ->  zero-ref    "field V01.Item1 (fldOffset=0x8)" P-INDEP
;* V09 tmp7         [V09    ] (  0,  0   )  struct (16) zero-ref    "Promoted implicit byref" <System.ValueTuple`2[int,System.__Canon]>
;  V10 cse0         [V10,T03] (  3,  3   )    long  ->  rax         "CSE #01: aggressive"
;
; Lcl frame size = 40

G_M33081_IG01:  ;; offset=0x0000
       sub      rsp, 40
       vxorps   xmm4, xmm4, xmm4
       vmovdqu  xmmword ptr [rsp+0x08], xmm4
       xor      eax, eax
       mov      qword ptr [rsp+0x18], rax
       mov      qword ptr [rsp+0x20], rcx
						;; size=26 bbWeight=1 PerfScore 4.83
G_M33081_IG02:  ;; offset=0x001A
       mov      rax, qword ptr [rcx+0x40]
       mov      rcx, qword ptr [rax]
       mov      qword ptr [rsp+0x08], rcx
						;; size=12 bbWeight=1 PerfScore 5.00
G_M33081_IG03:  ;; offset=0x0026
       vmovdqu  xmm0, xmmword ptr [rdx]
       vmovdqu  xmmword ptr [rsp+0x10], xmm0
						;; size=10 bbWeight=1 PerfScore 5.00
G_M33081_IG04:  ;; offset=0x0030
       mov      rcx, qword ptr [rsp+0x08]
       lea      rdx, bword ptr [rsp+0x08]
       xor      r8, r8
       cmp      rcx, qword ptr [rax+0x08]
       cmovne   rdx, r8
       test     rdx, rdx
       setne    al
       movzx    rax, al
						;; size=30 bbWeight=1 PerfScore 6.50
G_M33081_IG05:  ;; offset=0x004E
       add      rsp, 40
       ret      
						;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 83, prolog size 26, PerfScore 22.58, instruction count 21, allocated bytes for code 83 (MethodHash=0b7f7ec6) for method Program:Test[System.ValueTuple`2[int,System.__Canon],System.ValueTuple`2[int,System.__Canon]](System.ValueTuple`2[int,System.__Canon]):ubyte (FullOpts)
; ============================================================

With this PR + #103391 the disassembly for this function becomes

; Assembly listing for method C:Test[System.ValueTuple`2[int,System.__Canon],System.ValueTuple`2[int,System.__Canon]](System.ValueTuple`2[int,System.__Canon]):ubyte (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; FullOpts code
; optimized code
; rsp based frame
; partially interruptible
; No PGO data
; Final local variable assignments
;
;  V00 TypeCtx      [V00,T00] (  4,  4   )    long  ->  rcx         single-def
;* V01 arg0         [V01    ] (  0,  0   )   byref  ->  zero-ref    single-def
;# V02 OutArgs      [V02    ] (  1,  1   )  struct ( 0) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V03 tmp1         [V03    ] (  0,  0   )    long  ->  zero-ref    class-hnd exact "Single-def Box Helper" <System.ValueTuple`2[int,System.__Canon]>
;* V04 tmp2         [V04    ] (  0,  0   )   byref  ->  zero-ref    "ISINST eval op1"
;* V05 tmp3         [V05    ] (  0,  0   )     int  ->  zero-ref    "spilling qmarkNull"
;* V06 tmp4         [V06    ] (  0,  0   )  struct (24) zero-ref    do-not-enreg[SF] "stack allocated boxed value class temp" <System.Collections.Generic.KeyValuePair`2[long,System.ValueTuple`2[int,System.__Canon]]>
;* V07 tmp5         [V07    ] (  0,  0   )     ref  ->  zero-ref    "field V01.Item2 (fldOffset=0x0)" P-INDEP
;* V08 tmp6         [V08    ] (  0,  0   )     int  ->  zero-ref    "field V01.Item1 (fldOffset=0x8)" P-INDEP
;  V09 tmp7         [V09,T02] (  2,  2   )    long  ->  rcx         "V06.[000..008)"
;* V10 tmp8         [V10    ] (  0,  0   )     ref  ->  zero-ref    "V06.[008..016)"
;* V11 tmp9         [V11    ] (  0,  0   )     int  ->  zero-ref    "V06.[016..020)"
;* V12 tmp10        [V12    ] (  0,  0   )  struct (16) zero-ref    "Promoted implicit byref" <System.ValueTuple`2[int,System.__Canon]>
;  V13 cse0         [V13,T01] (  3,  3   )    long  ->  rax         "CSE #01: aggressive"
;
; Lcl frame size = 8

G_M43406_IG01:  ;; offset=0x0000
       push     rax
       mov      qword ptr [rsp], rcx
                                                ;; size=5 bbWeight=1 PerfScore 2.00
G_M43406_IG02:  ;; offset=0x0005
       mov      rax, qword ptr [rcx+0x40]
       mov      rcx, qword ptr [rax]
       cmp      rcx, qword ptr [rax+0x08]
       setne    al
       movzx    rax, al
                                                ;; size=17 bbWeight=1 PerfScore 8.25
G_M43406_IG03:  ;; offset=0x0016
       add      rsp, 8
       ret
                                                ;; size=5 bbWeight=1 PerfScore 1.25

; Total bytes of code 27, prolog size 5, PerfScore 11.50, instruction count 9, allocated bytes for code 27 (MethodHash=e76e5671) for method C:Test[System.ValueTuple`2[int,System.__Canon],System.ValueTuple`2[int,System.__Canon]](System.ValueTuple`2[int,System.__Canon]):ubyte (FullOpts)
; ============================================================

src/coreclr/jit/objectalloc.cpp

jakobbotsch · 2024-06-27T17:21:50Z

src/coreclr/jit/objectalloc.cpp

+                    if ((secondArgNode->OperIsLocal() || secondArgNode->OperIs(GT_LCL_ADDR)) &&
+                        !secondArgNode->TypeIs(TYP_REF))


What guarantees that secondArgNode in the secondArgNode->OperIsLocal() case is pointing to something stack allocated?

Just the rewritten type. I can validate the local num too I suppose.

Actually not quite this simple, I would need to track all locals that get rewritten. I think checking the type is ok.

We can safely do this transformation for all unbox helper calls, but if the box is on the heap it will just increase code size with no benefit.

It seems like doing this transformation during local morph would make its status as "avoiding address exposure" a bit more clear, but I think this is ok then.

src/coreclr/jit/objectalloc.cpp

jakobbotsch · 2024-06-27T17:27:48Z

src/coreclr/jit/objectalloc.cpp

+                            // flag this comma so we can find it later
+                            comma->gtFlags |= GTF_MAKE_CSE;


What is this about?

It's to enable the rewriting of IND(COMMA(x, y)) to COMMA(x, IND(y)) just below.

Removed in favor of more elaborate check on the IND side.

jakobbotsch · 2024-06-27T17:29:25Z

src/coreclr/jit/objectalloc.cpp

+                GenTreeIndir* const indir = tree->AsIndir();
+                GenTree* const      addr  = indir->Addr();
+
+                if (addr->OperIs(GT_COMMA) && ((addr->gtFlags & GTF_MAKE_CSE) != 0))


Can we look for the specific pattern rather than repurposing a flag like this?

I suppose we can check if the comma side effect child is the unbox type test helper.

Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

AndyAyersMS · 2024-06-28T02:10:32Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2024-06-28T02:10:42Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2024-06-28T05:52:07Z

/azp run runtime-coreclr jitstress

azure-pipelines · 2024-06-28T05:52:23Z

Azure Pipelines successfully started running 1 pipeline(s).

AndyAyersMS · 2024-06-28T14:43:55Z

Jitstress has possibly one novel failure, in CoreMangLib\system\span\RefStructWithSpan ... investigating.

jakobbotsch · 2024-06-28T14:47:42Z

Jitstress has possibly one novel failure, in CoreMangLib\system\span\RefStructWithSpan ... investigating.

That one might be #103624

AndyAyersMS · 2024-06-28T14:53:25Z

Jitstress has possibly one novel failure, in CoreMangLib\system\span\RefStructWithSpan ... investigating.

That one might be #103624

Seems likely, yes.

Any other feedback?

jakobbotsch · 2024-06-28T14:58:44Z

src/coreclr/jit/objectalloc.cpp

+                if (lhsType != newType)
+                {
+                    lhs->ChangeType(newType);
+                }
+
+                if (rhsType != newType)
+                {
+                    rhs->ChangeType(newType);
+                }
+
+                parent->ChangeType(newType);


It still seems odd to me to forcefully retype an arbitrary sibling node of an ancestor, but maybe all TYP_REF JIT IR nodes really do have equivalently typed TYP_BYREF nodes.

For example, this will change TYP_REF LCL_VAR(local whose LclVarDsc is TYP_REF) to TYP_BYREF LCL_VAR(local whose LclVarDsc is TYP_REF)... That will hit this assert:

runtime/src/coreclr/jit/lclvars.cpp

Lines 4691 to 4694 in fcc2f58

// Check that the LCL_VAR node has the same type as the underlying variable, save a few mismatches we allow.

assert(tree->TypeIs(varDsc->TypeGet(), genActualType(varDsc)) ||

(tree->TypeIs(TYP_BYREF) && (varDsc->TypeGet() == TYP_I_IMPL)) || // Created by inliner substitution.

(tree->TypeIs(TYP_INT) && (varDsc->TypeGet() == TYP_LONG))); // Created by "optNarrowTree".

Not sure if it will actually cause issues downstream, but it feels like we should insert some form of cast to maintain IR invariants. Potentially in quite a few places in object stack allocation. Anyway, something to think about, we don't need to block on this given that QMARKs have very restricted shapes.

Happy to reconsider ... I added this because of assertion failures but don't recall the specific case and now can't reproduce it. Not clear to me how the JIT could create a TYP_REF GT_COLON, at least not one that this code would see.

Here's the motivating example -- from the object stack allocation tests

[000035] DA-X------- * STORE_LCL_VAR ref V03 tmp3 [000034] ---X------- \--* QMARK ref [000025] ----------- if +--* EQ int [000024] ----------- | +--* LCL_VAR ref V02 tmp2 [000023] ----------- | \--* CNS_INT ref null [000033] ---X------- if \--* COLON ref [000031] ---X------- else +--* QMARK ref [000027] ---X------- if | +--* NE int [000026] #--X------- | | +--* IND long [000022] ----------- | | | \--* LCL_VAR ref V02 tmp2 [000021] H---------- | | \--* CNS_INT(h) long 0x7ffe34d5e970 class ObjectStackAllocation.SimpleClassB [000030] ----------- if | \--* COLON ref [000028] ----------- else | +--* LCL_VAR ref V02 tmp2 [000029] ----------- then | \--* CNS_INT ref null [000032] ----------- then \--* CNS_INT ref null

We stack allocate the ref class, and so retype tmp2 to byref. This propagates up and if we don't retype sibling [000029] we hit asserts.

I think it may be the case that the mistyped sibling must be null... I can check for that.

jakobbotsch

LGTM, a comment on why the retyping feels wrong to me.

AndyAyersMS · 2024-06-28T15:49:01Z

@MichalStrehovsky any feedback? Think you got added as a reviewer automatically.

AndyAyersMS · 2024-06-28T17:15:10Z

LGTM, a comment on why the retyping feels wrong to me.

Turns out we only need to retype nulls, hopefully that's a bit more palatable.

AndyAyersMS · 2024-06-28T19:30:23Z

CI seems stuck...

AndyAyersMS · 2024-06-28T21:39:05Z

@dotnet/dnceng is CI stuck? No evident progress here in 2 hours.

mmitche · 2024-06-28T21:40:09Z

@dotnet/dnceng is CI stuck? No evident progress here in 2 hours.

GitHub had an outage.

AndyAyersMS and others added 9 commits June 10, 2024 15:35

wip

e333c8f

post rebase fixes

5deb676

wip

877927a

revert layout changes, rely on KVP instead

0a7f3d4

trying to enable promotion

56500c3

Small fixes

f9e6e47

Update src/coreclr/jit/lclmorph.cpp

67402d4

Merge pull request #6 from jakobbotsch/stack-allocate-unescaped-boxes…

5a08c84

…-lclmorph Minor fixes

format

350d7ca

dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 12, 2024

dotnet-policy-service bot assigned AndyAyersMS Jun 12, 2024

jkotas reviewed Jun 12, 2024

View reviewed changes

avoid escaping nullcheck addrs

0328665

AndyAyersMS mentioned this pull request Jun 12, 2024

JIT should support custom ClassLayout instances with GC refs #103362

Open

jakobbotsch reviewed Jun 12, 2024

View reviewed changes

AndyAyersMS added 3 commits June 12, 2024 19:39

fix nullcheck handling

315916d

fix colon retyping

91b83d3

metrics, enable for ref classes too

bcc7c3a

jakobbotsch reviewed Jun 27, 2024

View reviewed changes

src/coreclr/jit/objectalloc.cpp Outdated Show resolved Hide resolved

jakobbotsch reviewed Jun 27, 2024

View reviewed changes

src/coreclr/jit/objectalloc.cpp Outdated Show resolved Hide resolved

jakobbotsch reviewed Jun 27, 2024

View reviewed changes

Apply suggestions from code review

8a077ea

Co-authored-by: Jan Kotas <jkotas@microsoft.com> Co-authored-by: Jakob Botsch Nielsen <Jakob.botsch.nielsen@gmail.com>

build-analysis bot mentioned this pull request Jun 28, 2024

System.IO.FileSystem.Tests.WorkItemExecution CI timeout on net9.0-osx-Debug-x64-Mono_Minijit_Debug-OSX.1200.Amd64.Open #101423

Open

more review feedback

dc873e5

AndyAyersMS added 2 commits June 27, 2024 19:45

fix

2597a44

fix typo in comment

504012d

jakobbotsch reviewed Jun 28, 2024

View reviewed changes

jakobbotsch approved these changes Jun 28, 2024

View reviewed changes

more surgical sibling type update for qmark/colon

7af3d98

AndyAyersMS closed this Jun 28, 2024

AndyAyersMS reopened this Jun 28, 2024

mmitche closed this Jun 28, 2024

mmitche reopened this Jun 28, 2024

build-analysis bot mentioned this pull request Jun 29, 2024

The "RestoreTask" task returned false but did not log an error dotnet/dnceng#3100

Open

3 tasks

		if ((secondArgNode->OperIsLocal() \|\| secondArgNode->OperIs(GT_LCL_ADDR)) &&
		!secondArgNode->TypeIs(TYP_REF))

		// flag this comma so we can find it later
		comma->gtFlags \|= GTF_MAKE_CSE;

	// Check that the LCL_VAR node has the same type as the underlying variable, save a few mismatches we allow.
	assert(tree->TypeIs(varDsc->TypeGet(), genActualType(varDsc)) \|\|
	(tree->TypeIs(TYP_BYREF) && (varDsc->TypeGet() == TYP_I_IMPL)) \|\| // Created by inliner substitution.
	(tree->TypeIs(TYP_INT) && (varDsc->TypeGet() == TYP_LONG))); // Created by "optNarrowTree".

Stack allocate unescaped boxes #103361

Are you sure you want to change the base?

Stack allocate unescaped boxes #103361

Conversation

AndyAyersMS commented Jun 12, 2024 • edited Loading

AndyAyersMS commented Jun 12, 2024

AndyAyersMS commented Jun 12, 2024

jakobbotsch commented Jun 12, 2024

AndyAyersMS commented Jun 12, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Jun 12, 2024

AndyAyersMS commented Jun 12, 2024

AndyAyersMS commented Jun 12, 2024

AndyAyersMS commented Jun 12, 2024

Choose a reason for hiding this comment

jakobbotsch commented Jun 13, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AndyAyersMS commented Jun 28, 2024

azure-pipelines bot commented Jun 28, 2024

AndyAyersMS commented Jun 28, 2024

azure-pipelines bot commented Jun 28, 2024

AndyAyersMS commented Jun 28, 2024

jakobbotsch commented Jun 28, 2024

AndyAyersMS commented Jun 28, 2024

Choose a reason for hiding this comment

jakobbotsch Jun 28, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jakobbotsch left a comment

Choose a reason for hiding this comment

AndyAyersMS commented Jun 28, 2024

AndyAyersMS commented Jun 28, 2024

AndyAyersMS commented Jun 28, 2024

AndyAyersMS commented Jun 28, 2024

mmitche commented Jun 28, 2024

AndyAyersMS commented Jun 12, 2024 •

edited

Loading

jakobbotsch commented Jun 13, 2024 •

edited

Loading

jakobbotsch Jun 28, 2024 •

edited

Loading