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

Track dependently-promoted fields when struct local is no longer referenced. #37280

Merged
merged 1 commit into from
Jun 9, 2020

Conversation

erozenfeld
Copy link
Member

This is a follow-up to #36918. It addresses one of the examples in #1007
where we remove a struct zero initialization but fail to clean up a dead
field assignment.

The change is not to mark a dependently promoted field as untracked
if we know that the struct local is no longer referenced.

I also addressed a couple of late cosmetic review comments from #36918.

No diffs in framework and benchmarks.

@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Jun 1, 2020
@erozenfeld
Copy link
Member Author

Example from #1007:

Example 4:

using System.Runtime.InteropServices;

public class C {
    [StructLayout(LayoutKind.Sequential, Size = 24)]
    public struct Buffer
    {
        public ulong Field1;
        public ulong Field2;
        public uint Field3;
    }
    
    public void M() {
        var buf = new Buffer();
        buf.Field1 = 2;
    }
}

Diffs after #36918 (optimization to remove zero inits):

G_M25775_IG01:
       4883EC18             sub      rsp, 24
-      C5F877               vzeroupper
       C5D857E4             vxorps   xmm4, xmm4
       C5F97F2424           vmovdqa  xmmword ptr [rsp], xmm4
       33C0                 xor      rax, rax
       4889442410           mov      qword ptr [rsp+10H], rax
                                                ;; bbWeight=1    PerfScore 3.83
G_M25775_IG02:
-      33C0                 xor      eax, eax
-      C5F857C0             vxorps   xmm0, xmm0
-      C5FA7F0424           vmovdqu  xmmword ptr [rsp], xmm0
-      4889442410           mov      qword ptr [rsp+10H], rax
       48C7042402000000     mov      qword ptr [rsp], 2
G_M25775_IG03:
       4883C418             add      rsp, 24
       C3                   ret

-; Total bytes of code 52, prolog size 23, PerfScore 14.27, (MethodHash=0c929b50) for method C:M()
+; Total bytes of code 33, prolog size 20, PerfScore 8.58, (MethodHash=0c929b50) for method C:M()

Diffs after this change:

G_M25775_IG01:
-      4883EC18             sub      rsp, 24
-      C5D857E4             vxorps   xmm4, xmm4
-      C5F97F2424           vmovdqa  xmmword ptr [rsp], xmm4
-      33C0                 xor      rax, rax
-      4889442410           mov      qword ptr [rsp+10H], rax
G_M25775_IG02:
-      48C7042402000000     mov      qword ptr [rsp], 2
G_M25775_IG03:
-      4883C418             add      rsp, 24
       C3                   ret

-; Total bytes of code 33, prolog size 20, PerfScore 8.58, (MethodHash=0c929b50) for method C:M()
+; Total bytes of code 1, prolog size 0, PerfScore 1.10, (MethodHash=0c929b50) for method C:M()

@erozenfeld
Copy link
Member Author

@dotnet/jit-contrib @CarolEidt PTAL

@erozenfeld erozenfeld requested a review from CarolEidt June 1, 2020 22:30
This is a follow-up to dotnet#36918. It addresses one of the examples in dotnet#1007
where we remove a struct zero initialization but fail to clean up a dead
field assignment.

The change is not to mark a dependently promoted field as untracked
if we know that the struct local is no longer referenced.

I also addressed a couple of late cosmetic review comments from dotnet#36918.

No diffs in framework and benchmarks.
@erozenfeld erozenfeld changed the title Track promoted fields when struct local is no longer referenced. Track dependently-promoted fields when struct local is no longer referenced. Jun 8, 2020
@erozenfeld
Copy link
Member Author

I pushed a change that fixes Linux arm test failures.
@CarolEidt @dotnet/jit-contrib PTAL

Copy link
Contributor

@CarolEidt CarolEidt left a comment

Choose a reason for hiding this comment

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

LGTM, with just one question for curiosity's sake.

}
// Fields of dependently promoted structs may be tracked. We shouldn't set lvMustInit on them since
// the whole parent struct will be initialized; however, lvLiveInOutOfHndlr should be set on them
// as appropriate.
Copy link
Contributor

Choose a reason for hiding this comment

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

If the field is dependently promoted (i.e. the whole struct is forced to live on the stack), then do we still need to mark them as lvLiveInOutOfHndlr since they'll always be defined/used to/from the stack?

Copy link
Member Author

Choose a reason for hiding this comment

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

With this change we may enregister a field of a dependently promoted struct if the struct is no longer used. See the change in lclvars.cpp: we don't set lvaSetVarDoNotEnregister on such fields.

@erozenfeld erozenfeld merged commit ee17aa6 into dotnet:master Jun 9, 2020
CarolEidt added a commit to CarolEidt/runtime that referenced this pull request Jun 10, 2020
@JulieLeeMSFT JulieLeeMSFT added this to the 5.0.0 milestone Jun 19, 2020
@ghost ghost locked as resolved and limited conversation to collaborators Dec 9, 2020
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants