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

[libraries-jitstress] System.Tests.UInt128Tests_GenericMath.TryReadLittleEndianUInt64Test fails in CI #104613

Closed
jakobbotsch opened this issue Jul 9, 2024 · 3 comments · Fixed by #104616
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci-optional Blocking optional rolling runs
Milestone

Comments

@jakobbotsch
Copy link
Member

Example pipeline run: https://dev.azure.com/dnceng-public/public/_build/results?buildId=734477&view=results
Example console log: https://helixre107v0xdcypoyl9e7f.blob.core.windows.net/dotnet-runtime-refs-pull-104593-merge-a1079025a96a477299/System.Runtime.Tests/1/console.6e9bbd2a.log?helixlogtype=result

I verified that the problem reproduces with the latest main. It looks caused by #104467, though it must have interfered with another PR since libraries-jitstress didn't hit this failure on that PR.

@dotnet-issue-labeler dotnet-issue-labeler bot added the needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners label Jul 9, 2024
@jakobbotsch jakobbotsch self-assigned this Jul 9, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 9, 2024
@jakobbotsch jakobbotsch added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI blocking-clean-ci-optional Blocking optional rolling runs and removed untriaged New issue has not been triaged by the area owner needs-area-label An area label is needed to ensure this gets routed to the appropriate area owners labels Jul 9, 2024
@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Jul 9, 2024
Copy link
Contributor

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

@jakobbotsch
Copy link
Member Author

The codegen for System.UInt128:op_LeftShift relies on the fact that the retbuffer does not alias alias any incoming implicit byref:

; Assembly listing for method System.UInt128:op_LeftShift(System.UInt128,int):System.UInt128 (FullOpts)
; Emitting BLENDED_CODE for X64 with AVX - Windows
; FullOpts code
; optimized code
; optimized using Unknown PGO
; rsp based frame
; partially interruptible
; with Unknown PGO: fgCalledCount is 160
; No PGO data
; 0 inlinees with PGO data; 2 single block inlinees; 0 inlinees without PGO data
; Final local variable assignments
;
;  V00 RetBuf       [V00,T02] (  3,  3   )   byref  ->   r8         single-def
;  V01 arg0         [V01,T00] (  3,  6   )   byref  ->  rdx         single-def
;  V02 arg1         [V02,T03] (  3,  3   )     int  ->  rcx         single-def
;  V03 loc0         [V03,T08] (  2,  0.52)    long  ->  rax        
;  V04 loc1         [V04,T07] (  1,  1   )     int  ->  [rsp+0x2C]  do-not-enreg[V] "GSCookie dummy"
;  V05 OutArgs      [V05    ] (  1,  1   )  struct (32) [rsp+0x00]  do-not-enreg[XS] addr-exposed "OutgoingArgSpace"
;* V06 tmp2         [V06    ] (  0,  0   )  struct (16) zero-ref    do-not-enreg[SF] ld-addr-op "NewObj constructor temp" <System.UInt128>
;* V07 tmp3         [V07    ] (  0,  0   )  struct (16) zero-ref    do-not-enreg[SF] ld-addr-op "NewObj constructor temp" <System.UInt128>
;* V08 tmp4         [V08    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;* V09 tmp5         [V09    ] (  0,  0   )    long  ->  zero-ref    "Inlining Arg"
;* V10 tmp6         [V10    ] (  0,  0   )    long  ->  zero-ref    "V06.[000..008)"
;* V11 tmp7         [V11    ] (  0,  0   )    long  ->  zero-ref    "V06.[008..016)"
;* V12 tmp8         [V12    ] (  0,  0   )    long  ->  zero-ref    "V07.[000..008)"
;* V13 tmp9         [V13    ] (  0,  0   )    long  ->  zero-ref    "V07.[008..016)"
;  V14 GsCookie     [V14    ] (  1,  1   )    long  ->  [rsp+0x30]  do-not-enreg[X] addr-exposed "GSSecurityCookie"
;  V15 tmp11        [V15,T04] (  9,  3.76)   byref  ->   r8         single-def "shadowVar"
;  V16 tmp12        [V16,T05] (  6,  2.52)   byref  ->  rdx         single-def "shadowVar"
;  V17 tmp13        [V17,T01] (  8,  5.52)     int  ->  rcx         "shadowVar"
;  V18 cse0         [V18,T06] (  5,  1.78)     int  ->  r10         multi-def "CSE #01"
;
; Lcl frame size = 56

G_M38089_IG01:
       sub      rsp, 56
       mov      rax, 0xD1FFAB1E
       mov      qword ptr [rsp+0x30], rax
       mov      eax, r8d
       mov      r8, rcx
       mov      ecx, eax
						;; size=27 bbWeight=1 PerfScore 2.25
G_M38089_IG02:
       and      ecx, 127
       test     cl, 64
       je       SHORT G_M38089_IG06
						;; size=8 bbWeight=1 PerfScore 1.50
G_M38089_IG03:
       xor      eax, eax
       mov      qword ptr [r8], rax ; zeroing ret buffer
       mov      r10d, ecx
       and      r10d, 63
       mov      rax, qword ptr [rdx] ; accessing parameter that may have aliased the ret buffer
       shlx     rax, rax, r10
       mov      qword ptr [r8+0x08], rax
       mov      rax, r8
       mov      rcx, 0xD1FFAB1E
       cmp      qword ptr [rsp+0x30], rcx
       je       SHORT G_M38089_IG04
       call     CORINFO_HELP_FAIL_FAST
						;; size=49 bbWeight=0.50 PerfScore 4.88
G_M38089_IG04:
       nop      
						;; size=1 bbWeight=0.50 PerfScore 0.12
G_M38089_IG05:
       add      rsp, 56
       ret      
						;; size=5 bbWeight=0.50 PerfScore 0.62
G_M38089_IG06:
       test     ecx, ecx
       je       SHORT G_M38089_IG10
						;; size=4 bbWeight=0.50 PerfScore 0.62
G_M38089_IG07:
       mov      r10d, ecx
       and      r10d, 63
       mov      rax, qword ptr [rdx]
       shlx     rax, rax, r10
       mov      qword ptr [r8], rax
       mov      rax, qword ptr [rdx+0x08]
       shlx     rax, rax, r10
       neg      ecx
       add      ecx, 64
       mov      rdx, qword ptr [rdx]
       shrx     rcx, rdx, rcx
       or       rax, rcx
       mov      qword ptr [r8+0x08], rax
       mov      rax, r8
       mov      rcx, 0xD1FFAB1E
       cmp      qword ptr [rsp+0x30], rcx
       je       SHORT G_M38089_IG08
       call     CORINFO_HELP_FAIL_FAST
						;; size=72 bbWeight=0.26 PerfScore 3.96
G_M38089_IG08:
       nop      
						;; size=1 bbWeight=0.26 PerfScore 0.07
G_M38089_IG09:
       add      rsp, 56
       ret      
						;; size=5 bbWeight=0.26 PerfScore 0.33
G_M38089_IG10:
       vmovups  xmm0, xmmword ptr [rdx]
       vmovups  xmmword ptr [r8], xmm0
       mov      rax, r8
       mov      rcx, 0xD1FFAB1E
       cmp      qword ptr [rsp+0x30], rcx
       je       SHORT G_M38089_IG11
       call     CORINFO_HELP_FAIL_FAST
						;; size=34 bbWeight=0.24 PerfScore 2.52
G_M38089_IG11:
       nop      
						;; size=1 bbWeight=0.24 PerfScore 0.06
G_M38089_IG12:
       add      rsp, 56
       ret      
						;; size=5 bbWeight=0.24 PerfScore 0.30

; Total bytes of code 212, prolog size 19, PerfScore 17.23, instruction count 57, allocated bytes for code 212 (MethodHash=095f6b36) for method System.UInt128:op_LeftShift(System.UInt128,int):System.UInt128 (FullOpts)
; ============================================================

We either need to disable implicit byref copy elision at the caller, or make sure that we do not reorder this zeroing of the return buffer with parameters. (We already have to take care we do not do this, since the ret buffer can arbitrarily alias byrefs.)

@jakobbotsch
Copy link
Member Author

I think it's more reasonable to go back to the old invariant (implicit byrefs do not alias anything), so I'll do the analysis to disable last-use copy elision if the argument overlaps the return buffer.

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Jul 9, 2024
The recent work to allow more retbuf definitions uncovered cases where
we would end up with the retbuffer aliasing an implicit byref argument.
The JIT does not handle this aliasing, which would require proper
`GTF_GLOB_REF` flags on the local nodes accesses implicit byrefs. Fix
the problem by disallowing last-use copy elision when the local would
alias the return buffer.

Fix dotnet#104613
@github-actions github-actions bot locked and limited conversation to collaborators Aug 9, 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 blocking-clean-ci-optional Blocking optional rolling runs
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant