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

Consume Roslyn with ref fields support #71498

Merged
merged 25 commits into from
Jul 2, 2022

Conversation

AaronRobinsonMSFT
Copy link
Member

@AaronRobinsonMSFT AaronRobinsonMSFT commented Jun 30, 2022

Moves to a Roslyn version that support ref field support and the scoped keyword.

Removes ByReference<T> from all runtimes.

Contributes to #63768

@dotnet-issue-labeler
Copy link

Note regarding the new-api-needs-documentation label:

This serves as a reminder for when your PR is modifying a ref *.cs file and adding/modifying public APIs, to please make sure the API implementation in the src *.cs file is documented with triple slash comments, so the PR reviewers can sign off that change.

@dotnet-issue-labeler
Copy link

I couldn't figure out the best area label to add to this PR. If you have write-permissions please help me learn by adding exactly one area label.

@ghost
Copy link

ghost commented Jun 30, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

Moves to a Roslyn version that support ref field support and the scoped keyword.

Removes ByReference<T> from all runtimes.

Author: AaronRobinsonMSFT
Assignees: AaronRobinsonMSFT
Labels:

area-Meta, CoreClr, Mono

Milestone: -

@AaronRobinsonMSFT
Copy link
Member Author

@AaronRobinsonMSFT I'm not seeing any interpreter issues on the tests. Is there anything else that needs fixing ?

@BrzVlad I think we've made it past most of the legs that were failing so I perhaps this is solved. I applied your suggestion and verified it locally fixed the issue I was hitting above - Thank you.

@BrzVlad or @lambdageek Can I get one of you to sign-off on the mono changes?

@AaronRobinsonMSFT AaronRobinsonMSFT merged commit 912ef01 into main Jul 2, 2022
@AaronRobinsonMSFT AaronRobinsonMSFT deleted the feature/csharp_ref_fields branch July 2, 2022 21:07
@EgorBo
Copy link
Member

EgorBo commented Jul 5, 2022

public Span<byte> Test(byte[] array) => array.AsSpan(0, 10);

codegen before this PR:

; Method Program:ArrayAsSpanStartLength(System.Byte[]):System.Span`1[Byte]:this
G_M63029_IG01:      
       4883EC28             sub      rsp, 40

G_M63029_IG02:            
       4D85C0               test     r8, r8
       741D                 je       SHORT G_M63029_IG04
       418378080A           cmp      dword ptr [r8+8], 10
       7216                 jb       SHORT G_M63029_IG04
       4983C010             add      r8, 16
       4C8902               mov      bword ptr [rdx], r8
       C742080A000000       mov      dword ptr [rdx+8], 10
       488BC2               mov      rax, rdx

G_M63029_IG03:         
       4883C428             add      rsp, 40
       C3                   ret      

G_M63029_IG04:          
       FF15247D2700         call     [System.ThrowHelper:ThrowArgumentOutOfRangeException()]
       CC                   int3     
; Total bytes of code: 45

Codegen after this PR:

; Method Program:ArrayAsSpanStartLength(System.Byte[]):System.Span`1[Byte]:this
G_M63029_IG01:  
       57                   push     rdi
       56                   push     rsi
       53                   push     rbx
       4883EC30             sub      rsp, 48
       C5F877               vzeroupper 
       488BDA               mov      rbx, rdx

G_M63029_IG02:             
       C5F857C0             vxorps   xmm0, xmm0
       C5FA7F442420         vmovdqu  xmmword ptr [rsp+20H], xmm0
       4D85C0               test     r8, r8
       7432                 je       SHORT G_M63029_IG04
       418378080A           cmp      dword ptr [r8+8], 10
       722B                 jb       SHORT G_M63029_IG04
       4983C010             add      r8, 16
       4C89442420           mov      bword ptr [rsp+20H], r8
       C74424280A000000     mov      dword ptr [rsp+28H], 10
       488BFB               mov      rdi, rbx
       488D742420           lea      rsi, bword ptr [rsp+20H]
       E8DF738B5F           call     CORINFO_HELP_ASSIGN_BYREF
       48A5                 movsq    
       488BC3               mov      rax, rbx

G_M63029_IG03:         
       4883C430             add      rsp, 48
       5B                   pop      rbx
       5E                   pop      rsi
       5F                   pop      rdi
       C3                   ret      

G_M63029_IG04:
       FF158CDB0B00         call     [System.ThrowHelper:ThrowArgumentOutOfRangeException()]
       CC                   int3     
; Total bytes of code: 85

perf-triage team (@DrewScoggins, @tannergooding, @kunalspathak) noticed that this PR caused a lot of perf regressions

@tannergooding
Copy link
Member

tannergooding commented Jul 5, 2022

Notably SuperPMI says "zero diffs" which is interesting. Possibly two changes not playing well together or maybe something SuperPMI doesn't catch?

@kunalspathak
Copy link
Member

or maybe something SuperPMI doesn't catch?

@BruceForstall

@jakobbotsch
Copy link
Member

Notably SuperPMI says "zero diffs" which is interesting. Possibly two changes not playing well together or maybe something SuperPMI doesn't catch?

Since all the significant changes here are on the VM side that is to be expected. SPMI will only reflect JIT changes.

@DrewScoggins
Copy link
Member

We are also seeing over 10% regressions in plaintext TE

image

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta runtime-coreclr specific to the CoreCLR runtime
Projects
None yet
Development

Successfully merging this pull request may close these issues.