Open jakobbotsch opened 1 year ago
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak See info in area-owners.md if you want to be subscribed.
Author: | jakobbotsch |
---|---|
Assignees: | - |
Labels: | `area-CodeGen-coreclr`, `untriaged` |
Milestone: | - |
The particular diff is:
@@ -9,13 +9,13 @@
; 1 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
-; V00 arg0 [V00,T00] ( 3, 3 ) int -> rax single-def
+; V00 arg0 [V00,T00] ( 3, 3 ) int -> r8 single-def
; V01 arg1 [V01,T01] ( 3, 3 ) bool -> rdx single-def
-; V02 loc0 [V02,T02] ( 3, 2 ) int -> r8
+; V02 loc0 [V02,T02] ( 4, 4.00) int -> [rsp+24H]
;* V03 loc1 [V03 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op
; V04 OutArgs [V04 ] ( 1, 1 ) lclBlk (32) [rsp+00H] "OutgoingArgSpace"
;* V05 tmp1 [V05 ] ( 0, 0 ) struct ( 8) zero-ref ld-addr-op "NewObj constructor temp"
-; V06 tmp2 [V06,T03] ( 2, 2.00) long -> rcx "Inline return value spill temp"
+; V06 tmp2 [V06,T03] ( 2, 2.00) long -> rdx "Inline return value spill temp"
;* V07 tmp3 [V07 ] ( 0, 0 ) ref -> zero-ref class-hnd single-def "dup spill"
;* V08 tmp4 [V08,T04] ( 0, 0 ) ref -> zero-ref single-def
;* V09 tmp5 [V09 ] ( 0, 0 ) ref -> zero-ref single-def V03.m_type(offs=0x00) P-INDEP "field V03.m_type (fldOffset=0x0)"
@@ -23,35 +23,32 @@
;
; Lcl frame size = 40
-G_M51023_IG01: ; bbWeight=1, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, nogc <-- Prolog IG
+G_M51023_IG01: ; bbWeight=1.00, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, nogc <-- Prolog IG
sub rsp, 40
- mov eax, ecx
- ;; size=6 bbWeight=1 PerfScore 0.50
-G_M51023_IG02: ; bbWeight=1, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, isz
- mov r8d, 16
+ mov r8d, ecx
+ ;; size=7 bbWeight=1.00 PerfScore 0.50
+G_M51023_IG02: ; bbWeight=1.00, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
+ mov ecx, 16
+ mov eax, 80
test dl, dl
- jne SHORT G_M51023_IG05
- ;; size=10 bbWeight=1 PerfScore 1.50
-G_M51023_IG03: ; bbWeight=1.00, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref
- mov rcx, 0xD1FFAB1E
- mov rcx, qword ptr [rcx]
- mov edx, eax
+ cmovne ecx, eax
+ mov dword ptr [rsp+24H], ecx
+ mov rdx, 0xD1FFAB1E
+ mov rdx, qword ptr [rdx]
+ mov rcx, rdx
+ mov edx, r8d
+ mov r8d, dword ptr [rsp+24H]
call <unknown method>
; gcrRegs +[rax]
; gcr arg pop 0
nop
- ;; size=21 bbWeight=1.00 PerfScore 3.75
-G_M51023_IG04: ; bbWeight=1.00, epilog, nogc, extend
+ ;; size=49 bbWeight=1.00 PerfScore 7.00
+G_M51023_IG03: ; bbWeight=1.00, epilog, nogc, extend
add rsp, 40
ret
;; size=5 bbWeight=1.00 PerfScore 1.25
-G_M51023_IG05: ; bbWeight=0, gcVars=0000000000000000 {}, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, gcvars, byref, isz
- ; gcrRegs -[rax]
- mov r8d, 80
- jmp SHORT G_M51023_IG03
- ;; size=8 bbWeight=0 PerfScore 0.00
For basic blocks that we think are run rarely it is very likely that we should not if-convert them.
Is this because the If conversion is causing that block to effectively always get executed?
Is this because the If conversion is causing that block to effectively always get executed?
Yeah, exactly. The JIT also has special handling to place these blocks separately, away from the hot code (as you can see in the diff), LSRA knows to pessimize register allocation in those blocks if that is better for other blocks, etc.
If all blocks that contribute to if-conversion are RunRarely then it would be ok? It's just if some are and some are not that there is a possible concern?
If all blocks that contribute to if-conversion are RunRarely then it would be ok? It's just if some are and some are not that there is a possible concern?
Probably so, but it really depends on why they are all marked run rarely. If conversion has both upside and downside, so if we don't know anything about relative likelihoods in some subgraph, perhaps it is better to avoid the downside.
@jakobbotsch, assigning to you now. Please feel free to reassign.
Fixing this actually results in quite a few negative diffs:
Diffs are based on 1,564,337 contexts (467,626 MinOpts, 1,096,711 FullOpts).
MISSED contexts: 2,250 (0.14%)
Checking the diffs it's quite clear that the JIT has a tendency to mark blocks as run rarely based on very little evidence, e.g. Math.Min
in benchmarks.run_pgo has a fgCalledCount = 4
and still has a block marked as rarely run. Similarly for System.Int32.CompareTo
.
I think I will move this item to future for now.
For basic blocks that we think are run rarely it is very likely that we should not if-convert them.
Noticed this due to a diff in
System.GC:g__AllocateNewUninitializedArray|66_0[ubyte](int,bool):ubyte[]
over in #81267.cc @dotnet/jit-contrib @a74nh