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

Order of initialization in async kickoff method with params blocks Jit zero init eliding #45295

Closed
benaadams opened this issue Jun 18, 2020 · 1 comment · Fixed by #45262
Closed
Labels
Area-Compilers Bug Code Gen Quality Room for improvement in the quality of the compiler's generated code
Milestone

Comments

@benaadams
Copy link
Member

benaadams commented Jun 18, 2020

Currently Rosyln zeros the the AsyncMethodBuilder field with .locals init and then assigns the result of .Create to that field.

When .Create is inlined it is also zeros; so the assignment is a second set of zeroing.

The Jit has an optimization where it will remove the second set of zeroing dotnet/runtime#36918 ("Optimization to remove redundant zero initializations"); however this only kicks in if there aren't any preceding non-zero assignments.

e.g.

using System.IO.Pipelines;

public async ValueTask<ReadResult> OuterMethod(object o)
{
    return await InnerMethod(o);
    
    static async ValueTask<ReadResult> InnerMethod(object o) 
    {
        return default;
    }
}
G_M51821_IG01:
       push     rsi
       sub      rsp, 144
       vzeroupper                                ; zero
       xor      rax, rax                         ;  |
       mov      qword ptr [rsp+28H], rax         ;  |
       vxorps   xmm4, xmm4                       ;  |
       mov      rax, -96                         ;  |
       vmovdqa  xmmword ptr [rsp+rax+90H], xmm4  ;  |
       vmovdqa  xmmword ptr [rsp+rax+A0H], xmm4  ;  |
       vmovdqa  xmmword ptr [rsp+rax+B0H], xmm4  ;  |
       add      rax, 48                          ;  |
       jne      SHORT  -5 instr                  ;  V
       mov      rsi, rdx
						;; bbWeight=1    PerfScore 8.58
G_M51821_IG02:
       mov      gword ptr [rsp+28H], r8      ; save param ** blocks redundant zero elimination **
       xor      ecx, ecx                     ; re-zero
       vxorps   xmm0, xmm0                   ;    |
       vmovdqu  xmmword ptr [rsp+38H], xmm0  ;    |
       vmovdqu  xmmword ptr [rsp+48H], xmm0  ;    |
       mov      qword ptr [rsp+58H], rcx     ;    V
       mov      dword ptr [rsp+30H], -1
       lea      rcx, [rsp+28H]
       call     System.Runtime.CompilerServices.AsyncMethodBuilderCore:Start(byref)
       lea      rcx, bword ptr [rsp+38H]
       mov      rdx, rsi
       call     System.Runtime.CompilerServices.AsyncValueTaskMethodBuilder`1[ReadResult][System.IO.Pipelines.ReadResult]:get_Task():System.Threading.Tasks.ValueTask`1[ReadResult]:this
       mov      rax, rsi

If the call to .Create() was moved to the first step; prior to saving parameters to the fields so it should not block the redundant zero elimination optimization.

From dotnet/runtime#2325 (comment)

Regarding your second example where we first assign to a gc field and then initialize the struct field: currently my optimization doesn't handle that. It's possible to extend it to handle that case but that would require keeping track of field initializations and may be a little tricky.

However, now the answer to your question

Or would changing it to move all the zeroing above that first assignment to non-zero (which would be a much more complex refactoring in Roslyn) help?

is yes.

If the struct field zero initialization was done before the first assignment, it would be eliminated by the new optimization.

@RikkiGibson RikkiGibson added Area-Compilers Code Gen Quality Room for improvement in the quality of the compiler's generated code labels Jun 18, 2020
@jaredpar jaredpar added Bug help wanted The issue is "up for grabs" - add a comment if you are interested in working on it labels Jun 22, 2020
@jaredpar jaredpar added this to the 16.8 milestone Jun 22, 2020
@benaadams
Copy link
Member Author

Resolved by #45262

@jinujoseph jinujoseph removed the help wanted The issue is "up for grabs" - add a comment if you are interested in working on it label Jul 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area-Compilers Bug Code Gen Quality Room for improvement in the quality of the compiler's generated code
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants