[RyuJIT] Unnecessary register moves from Unsafe.AsRef<T>

[fiigii]

According to VTune characterization, ParseHeaders is one of the hot functions in TechEmpower-Plaintext scenario.

ParseHeaders invokes inlined DangerousGetPinnableReference twice, which generates roundtrip mov sequences from inlined Unsafe.AsRef<T>:

IN0056: 000203 call     System.IntPtr:ToPointer():long:this
IN0057: 000208 mov      rdi, rax
IN0058: 00020B mov      rax, rdi
IN0059: 00020E mov      rdi, rax
IN005a: 000211 jmp      SHORT G_M11292_IG13
......
IN0080: 0002BE call     System.IntPtr:ToPointer():long:this
IN0081: 0002C3 mov      rdi, rax
IN0082: 0002C6 mov      rax, rdi
IN0083: 0002C9 mov      rdi, rax
IN0084: 0002CC mov      eax, dword ptr [V48 rbp-198H]
IN0085: 0002D2 movsxd   rax, eax

These redundant moves are from Unsafe.AsRef<T> source code:

  .method public hidebysig static !!T& AsRef<T>(void* source) cil managed aggressiveinlining
  {
        .custom instance void System.Runtime.Versioning.NonVersionableAttribute::.ctor() = ( 01 00 00 00 )
        .locals (int32&)
        .maxstack 1
        ldarg.0
        // Roundtrip via a local to avoid type mismatch on return that the JIT inliner chokes on.
        stloc.0
        ldloc.0
        ret
  } // end of method Unsafe::AsRef

@jkotas 's example shows that this roundtrip has been correctly eliminated by the compiler, but it appears in the generated code of Span.DangerousGetPinnableReference.

I will look into providing a solution.

[fiigii]

cc @russellhadley @RussKeldorph

[AndyAyersMS]

Looks similar to #8011 (see dotnet/corefxlab#1479 for some detailed notes). If so, it is fallout from how pinning is described in the IL.

[fiigii]

@AndyAyersMS I think it is different. Here is the JIT dump.
During importing UnSafe.AsRef<T>, three variables (V50, V51, and V52) are introduced at ASG, and the IR is one-to-one maping to the source code.

*************** In impImport() for System.Runtime.CompilerServices.Unsafe:AsRef(long):byref

impImportBlockPending for BB85

Importing BB85 (PC=000) of 'System.Runtime.CompilerServices.Unsafe:AsRef(long):byref'
    [ 0]   0 (0x000) ldarg.0
lvaGrabTemp returning 51 (V51 tmp27) called for Inlining Arg.

    [ 1]   1 (0x001) stloc.0
lvaGrabTemp returning 52 (V52 tmp28) (a long lifetime temp) called for Inline stloc first use temp.


               [000971] ------------             *  STMT      void  
               [000968] ------------             |  /--*  LCL_VAR   byref  V51 tmp27        
               [000970] -A----------             \--*  ASG       byref 
               [000969] D------N----                \--*  LCL_VAR   byref  V52 tmp28        

    [ 0]   2 (0x002) ldloc.0
    [ 1]   3 (0x003) ret

    Inlinee Return expression (before normalization)  =>
               [000972] ------------             *  LCL_VAR   byref  V52 tmp28        


               [000975] ------------             *  STMT      void  
               [000972] ------------             |  /--*  LCL_VAR   byref  V52 tmp28        
               [000974] -A----------             \--*  ASG       byref 
               [000973] D------N----                \--*  LCL_VAR   byref  V50 tmp26        


    Inlinee Return expression (after normalization) =>
               [000976] ------------             *  LCL_VAR   byref  V50 tmp26       

Then these three variables and assignments are retained all through codegen:

Generating: N503 ( 17, 10) [000941] --C-G-------      t941 = *  CALL      long   System.IntPtr.ToPointer $8c3
							Byref regs: 00000080 {rdi} => 00000000 {}
							Call: GCvars=0000000000000000000000000000000000000000 {V03 V04 V05}, gcrefRegs=0000C008 {rbx r14 r15}, byrefRegs=00000000 {}
IN0080:        call     System.IntPtr:ToPointer():long:this
                                                             /--*  t941   long   
Generating: N505 ( 17, 10) [000978] DA--G-------             *  STORE_LCL_VAR long   V51 tmp27        d:3 rdi REG rdi
IN0081:        mov      rdi, rax
							V51 in reg rdi is becoming live  [000978]
							Live regs: 0000F008 {rbx r12 r13 r14 r15} => 0000F088 {rbx rdi r12 r13 r14 r15}
							Live vars: {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V234 V235 V236} => {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V51 V234 V235 V236}
genIPmappingAdd: ignoring duplicate IL offset 0x8c
Generating: N507 (  1,  3) [000971] ------------                IL_OFFSET void   IL offset: 0x8c REG NA
Generating: N509 (  1,  1) [000968] ------------      t968 =    LCL_VAR   byref  V51 tmp27        u:3 rdi (last use) REG rdi $8c3
                                                             /--*  t968   byref  
Generating: N511 (  1,  3) [000970] DA----------             *  STORE_LCL_VAR byref  V52 tmp28        d:3 rax REG rax
							V51 in reg rdi is becoming dead  [000968]
							Live regs: 0000F088 {rbx rdi r12 r13 r14 r15} => 0000F008 {rbx r12 r13 r14 r15}
							Live vars: {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V51 V234 V235 V236} => {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V234 V235 V236}
IN0082:        mov      rax, rdi
							V52 in reg rax is becoming live  [000970]
							Live regs: 0000F008 {rbx r12 r13 r14 r15} => 0000F009 {rax rbx r12 r13 r14 r15}
							Live vars: {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V234 V235 V236} => {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V52 V234 V235 V236}
							Byref regs: 00000000 {} => 00000001 {rax}
genIPmappingAdd: ignoring duplicate IL offset 0x8c
Generating: N513 (  1,  3) [000975] ------------                IL_OFFSET void   IL offset: 0x8c REG NA
Generating: N515 (  1,  1) [000972] ------------      t972 =    LCL_VAR   byref  V52 tmp28        u:3 rax (last use) REG rax $8c3
                                                             /--*  t972   byref  
Generating: N517 (  1,  3) [000974] DA----------             *  STORE_LCL_VAR byref  V50 tmp26        d:3 rdi REG rdi
							V52 in reg rax is becoming dead  [000972]
							Live regs: 0000F009 {rax rbx r12 r13 r14 r15} => 0000F008 {rbx r12 r13 r14 r15}
							Live vars: {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V52 V234 V235 V236} => {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V234 V235 V236}
							Byref regs: 00000001 {rax} => 00000000 {}
IN0083:        mov      rdi, rax
							V50 in reg rdi is becoming live  [000974]
							Live regs: 0000F008 {rbx r12 r13 r14 r15} => 0000F088 {rbx rdi r12 r13 r14 r15}
							Live vars: {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V234 V235 V236} => {V00 V03 V04 V05 V08 V09 V11 V13 V18 V48 V50 V234 V235 V236}
							Byref regs: 00000000 {} => 00000080 {rdi}

[AndyAyersMS]

There is still an initial long->byref assignment which may block subsequent simplification.

Unsafe.AsRef shouldn't need the ldarg / stloc / ldloc workaround anymore. The jit issue should have been fixed by dotnet/coreclr#11218 (let me know if not).

Removing this workaround should remove the latter two movs.

[jkotas]

Unsafe.AsRef shouldn't need the ldarg / stloc / ldloc workaround anymore

It still needs it when running on .NET Framework. The build is shared between all runtimes right now. To remove the workaround, we should add netcoreapp-specific build and remove the workaround in that build only.

[nietras]

we should add netcoreapp-specific build and remove the workaround in that build only

@jkotas How would that be done practically for the IL source of Unsafe? Can you if/def in IL?

[benaadams]

in project file; with source inludes/excludes?

[nietras]

@benaadams that would imply almost duplicating IL file as far as I understand, with maintenance issues. Just curious because I wasn't sure how this could be done in a good way with ilasm...

[jkotas]

ilasm has defines and ifdefs. It should be possible to build the fix on top of the existing include files: https://github.com/dotnet/corefx/tree/master/src/System.Runtime.CompilerServices.Unsafe/src/include .

[nietras]

I did not know you could do this, good to know, thanks ☺️

…

On Aug 19, 2017 17:18, "Jan Kotas" ***@***.***> wrote: ilasm has defines and ifdefs. It should be possible to build the fix on top of the existing include files: https://github.com/dotnet/ corefx/tree/master/src/System.Runtime.CompilerServices.Unsafe/src/include . — You are receiving this because you commented. Reply to this email directly, view it on GitHub <https://github.com/dotnet/coreclr/issues/13341#issuecomment-323529222>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AKTG77uX-jxGZzGMRSziJWEtc_BcZY-wks5sZvytgaJpZM4O1AgP> .

[nietras]

FYI I have started looking at a PR for this, as it seems like a small change. My plan is something like:

• Add preprocessor define e.g. NETCORE20 to src\System.Runtime.CompilerServices.Unsafe\src\include\netstandard\coreassembly.h (or new file??)
• Add ifdef in il to remove the code in question when netcore.
• Probably need to add netcoreapp specific build? How about packaging?

Is that it? Let me know if I am completely off here and if anything is missing? Not sure how we could test this if needed?

cc: @karelz