Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

JIT: optimize some cases of unused structs#18819

Merged
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:Fix18710v2
Jul 13, 2018
Merged

JIT: optimize some cases of unused structs#18819
AndyAyersMS merged 2 commits into
dotnet:masterfrom
AndyAyersMS:Fix18710v2

Conversation

@AndyAyersMS
Copy link
Copy Markdown
Member

In some cases CSC will use ldobj; pop to null check a pointer to
struct. This change avoids copying the struct value for such constructs.

Codegen may still redundantly null check, if there are multiple such checks
in a method.

Fixes #18710

In some cases CSC will use `ldobj; pop` to null check a pointer to
struct. This change avoids copying the struct value for such constructs.

Codegen may still redundantly null check, if there are multiple such checks
in a method.

Fixes #18710
@AndyAyersMS
Copy link
Copy Markdown
Member Author

@briansull PTAL
cc @dotnet/jit-contrib

No diffs in fx for crossgen or pmi. Possibly because CSC will sometimes use ldfld instead which is less costly and easier to dead code.

Diffs on example code from #18710 (added here as tests):

Summary:
(Lower is better)
Total bytes of diff: -216 (-25.50% of base)
    diff is an improvement.
Top file improvements by size (bytes):
        -194 : structpop2.dasm (-38.42% of base)
         -22 : structpop.dasm (-6.43% of base)
2 total files with size differences (2 improved, 0 regressed), 0 unchanged.
Top method improvements by size (bytes):
        -194 : structpop2.dasm - P:TestByPtr(long):int
         -22 : structpop.dasm - P:TestByPtr(long):int
2 total methods with size differences (2 improved, 0 regressed), 11 unchanged.

For instance:

 ; Assembly listing for method P:TestByPtr(long):int
 ; Emitting BLENDED_CODE for X64 CPU with AVX
 ; optimized code
 ; rsp based frame
 ; partially interruptible
 ; Final local variable assignments
 ;
-;  V00 arg0         [V00,T00] (  7,  7   )    long  ->  rcx
+;  V00 arg0         [V00,T00] (  5,  5   )    long  ->  rcx
 ;  V01 loc0         [V01,T02] (  2,  2   )     int  ->  rax
 ;  V02 OutArgs      [V02    ] (  1,  1   )  lclBlk (32) [rsp+0x00]
 ;  V03 tmp1         [V03,T01] (  2,  4   )     int  ->  rdx
-;  V04 tmp2         [V04    ] (  3,  6   )  struct (16) [rsp+0x28]   do-not-enreg[XS] addr-exposed
-;  V05 tmp3         [V05    ] (  1,  1   )     int  ->  [rsp+0x28]   do-not-enreg[X] addr-exposed V04.F1(offs=0x00) P-DEP
-;  V06 tmp4         [V06    ] (  1,  1   )     int  ->  [rsp+0x2C]   do-not-enreg[X] addr-exposed V04.F2(offs=0x04) P-DEP
-;  V07 tmp5         [V07    ] (  1,  1   )     int  ->  [rsp+0x30]   do-not-enreg[X] addr-exposed V04.F3(offs=0x08) P-DEP
 ;
-; Lcl frame size = 56
+; Lcl frame size = 40
 G_M47241_IG01:
-       sub      rsp, 56
+       sub      rsp, 40
 G_M47241_IG02:
        mov      edx, dword ptr [rcx]
        mov      eax, dword ptr [rcx+4]
-       mov      r8d, dword ptr [rcx]
-       mov      dword ptr [rsp+28H], r8d
-       mov      r8d, dword ptr [rcx+4]
-       mov      dword ptr [rsp+2CH], r8d
-       mov      ecx, dword ptr [rcx+8]
-       mov      dword ptr [rsp+30H], ecx
+       cmp      dword ptr [rcx], ecx
        mov      ecx, edx
        mov      edx, eax
        call     P:Do(int,int):int
        nop
 G_M47241_IG03:
-       add      rsp, 56
+       add      rsp, 40
        ret
-; Total bytes of code 48, prolog size 4 for method P:TestByPtr(long):int
+; Total bytes of code 26, prolog size 4 for method P:TestByPtr(long):int

Comment thread src/jit/importer.cpp
op1 = impGetStructAddr(op1, clsHnd, (unsigned)CHECK_SPILL_ALL, false);
// If the value being produced comes from loading
// via an underlying address, just null check the address.
if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ))
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, any volatile concerns?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, maybe so. Thanks for bringing that up.

A quick glance suggests the jit won't even record volatile prefixes for ldobj. Need to dig in some more.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, this case is for structs. It's not clear to me if volatile applies to structs. ECMA-335 doesn't seem to say that volatile is not allowed on structs but I don't think I've ever seen code using that.

But then it looks like the importer already drops popped volatile reads - #6172

@AndyAyersMS
Copy link
Copy Markdown
Member Author

Looks like CI is still not running normally.

@AndyAyersMS AndyAyersMS reopened this Jul 10, 2018
@briansull
Copy link
Copy Markdown

Do you know what is up with "Windows_NT x64 Checked CoreFX Tests"

I am also seeing one test in that leg report a failure as well.

@CarolEidt
Copy link
Copy Markdown

Do you know what is up with "Windows_NT x64 Checked CoreFX Tests"

I opened #18840 for this yesterday, which has now been fixed with #18844.

@AndyAyersMS
Copy link
Copy Markdown
Member Author

@briansull did you have any feedback?

Copy link
Copy Markdown

@briansull briansull left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine to me

@AndyAyersMS AndyAyersMS merged commit dc7492e into dotnet:master Jul 13, 2018
@AndyAyersMS AndyAyersMS deleted the Fix18710v2 branch July 13, 2018 20:21
AndyAyersMS added a commit to AndyAyersMS/coreclr that referenced this pull request Jul 16, 2018
With the advent of dotnet#18819 we may now see GT_NULLCHECK nodes with operands
that also can cause exceptions.

Handle this in value numbering.

Closes #18937.
AndyAyersMS added a commit that referenced this pull request Jul 17, 2018
With the advent of #18819 we may now see GT_NULLCHECK nodes with operands
that also can cause exceptions.

Handle this in value numbering.

Closes #18937.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants