JIT: Fix invalid last use propagation for implicit byrefs#80489
Merged
jakobbotsch merged 2 commits intodotnet:mainfrom Jan 11, 2023
Merged
JIT: Fix invalid last use propagation for implicit byrefs#80489jakobbotsch merged 2 commits intodotnet:mainfrom
jakobbotsch merged 2 commits intodotnet:mainfrom
Conversation
ghost
added
the
area-CodeGen-coreclr
|
Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch, @kunalspathak |
Member
Author
|
A lot of the regressions look like: @@ -7,7 +7,7 @@
; 0 inlinees with PGO data; 1 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
-; V00 arg0 [V00,T00] ( 4, 8 ) byref -> rcx ld-addr-op single-def
+; V00 arg0 [V00,T00] ( 4, 8 ) byref -> r8 ld-addr-op single-def
; V01 OutArgs [V01 ] ( 1, 1 ) lclBlk (32) [rsp+00H] "OutgoingArgSpace"
;* V02 tmp1 [V02 ] ( 0, 0 ) struct (16) zero-ref "impAppendStmt"
;* V03 tmp2 [V03 ] ( 0, 0 ) ref -> zero-ref V07._items(offs=0x00) P-INDEP "field V00._items (fldOffset=0x0)"
@@ -15,27 +15,35 @@
;* V05 tmp4 [V05 ] ( 0, 0 ) ref -> zero-ref V02._items(offs=0x00) P-INDEP "field V02._items (fldOffset=0x0)"
;* V06 tmp5 [V06 ] ( 0, 0 ) int -> zero-ref V02._length(offs=0x08) P-INDEP "field V02._length (fldOffset=0x8)"
;* V07 tmp6 [V07 ] ( 0, 0 ) struct (16) zero-ref "Promoted implicit byref"
+; V08 tmp7 [V08 ] ( 2, 4 ) struct (16) [rsp+28H] do-not-enreg[XS] addr-exposed "by-value struct argument"
;
-; Lcl frame size = 40
+; Lcl frame size = 56
G_M45936_IG01: ; bbWeight=1, gcrefRegs=00000000 {}, byrefRegs=00000000 {}, byref, nogc <-- Prolog IG
- sub rsp, 40
- ;; size=4 bbWeight=1 PerfScore 0.25
-G_M45936_IG02: ; bbWeight=1, gcrefRegs=00000000 {}, byrefRegs=00000002 {rcx}, byref
- ; byrRegs +[rcx]
- mov r8d, dword ptr [rcx+08H]
+ sub rsp, 56
+ vzeroupper
+ mov r8, rcx
+ ; byrRegs +[r8]
+ ;; size=10 bbWeight=1 PerfScore 1.50
+G_M45936_IG02: ; bbWeight=1, gcrefRegs=00000000 {}, byrefRegs=00000100 {r8}, byref, nogc
+ vmovdqu xmm0, xmmword ptr [r8]
+ vmovdqu xmmword ptr [rsp+28H], xmm0
+ ;; size=11 bbWeight=1 PerfScore 5.00
+G_M45936_IG03: ; bbWeight=1, extend
+ lea rcx, [rsp+28H]
+ mov r8d, dword ptr [r8+08H]
+ ; byrRegs -[r8]
xor edx, edx
call [Microsoft.CodeAnalysis.Collections.SegmentedArray:Reverse[ubyte](Microsoft.CodeAnalysis.Collections.SegmentedArray`1[ubyte],int,int)]
- ; byrRegs -[rcx]
; gcr arg pop 0
nop
- ;; size=13 bbWeight=1 PerfScore 5.50
-G_M45936_IG03: ; bbWeight=1, epilog, nogc, extend
- add rsp, 40
+ ;; size=18 bbWeight=1 PerfScore 6.00
+G_M45936_IG04: ; bbWeight=1, epilog, nogc, extend
+ add rsp, 56
ret
;; size=5 bbWeight=1 PerfScore 1.25
-; Total bytes of code 22, prolog size 4, PerfScore 9.20, instruction count 7, allocated bytes for code 22 (MethodHash=d0364c8f) for method Microsoft.CodeAnalysis.Collections.SegmentedArray:Reverse[ubyte](Microsoft.CodeAnalysis.Collections.SegmentedArray`1[ubyte])
+; Total bytes of code 44, prolog size 7, PerfScore 18.15, instruction count 12, allocated bytes for code 44 (MethodHash=d0364c8f) for method Microsoft.CodeAnalysis.Collections.SegmentedArray:Reverse[ubyte](Microsoft.CodeAnalysis.Collections.SegmentedArray`1[ubyte])
; ============================================================Notice that we have another later argument accessing a field. It would still be legal to omit the copy here though, but it would take some more analysis to prove. It seems doable though. |
Member
Author
|
Diffs. Regressions as expected due to fewer omitted copies. cc @dotnet/jit-contrib PTAL @AndyAyersMS |
AndyAyersMS
approved these changes
Jan 11, 2023
Member
|
With these commits (2ca7cf7...6aaaaaa) aspnet apps crash at runtime in the benchmarks CI. I am flagging the PR which had the potential to explain it. Crank command if you can run with and without this PR changes: |
Member
Author
|
cc @tannergooding, can you check if this is due to #80242? I think it is unlikely this PR is the cause, and I currently don't have access to run crank so I cannot double check. |
Member
|
We isolated the issue in the GC changes, please disregard my comment. |
ghost
locked as resolved and limited conversation to collaborators
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fix #80488