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

Constant propagation should preserve field sequences #64876

Closed
SingleAccretion opened this issue Feb 6, 2022 · 3 comments · Fixed by #78616
Closed

Constant propagation should preserve field sequences #64876

SingleAccretion opened this issue Feb 6, 2022 · 3 comments · Fixed by #78616
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI
Milestone

Comments

@SingleAccretion
Copy link
Contributor

This is a CQ-only issue. We can see cases where (local) constant propagation propagates known addresses of statics and loses field sequences for them. This causes VN to be more pessimistic about those addresses than is necessary.

Fixing this would require saving the field sequence alongside the constant itself in assertions. Unfortunately, currently there is no space available, so some refactoring would be required to make sure there would be no memory consumption regression from this change.

@dotnet-issue-labeler
Copy link

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

@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Feb 6, 2022
@SingleAccretion SingleAccretion added area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI and removed untriaged New issue has not been triaged by the area owner labels Feb 6, 2022
@ghost
Copy link

ghost commented Feb 6, 2022

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

Issue Details

This is a CQ-only issue. We can see cases where (local) constant propagation propagates known addresses of statics and loses field sequences for them. This causes VN to be more pessimistic about those addresses than is necessary.

Fixing this would require saving the field sequence alongside the constant itself in assertions. Unfortunately, currently there is no space available, so some refactoring would be required to make sure there would be no memory consumption regression from this change.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@EgorBo
Copy link
Member

EgorBo commented Nov 20, 2022

    struct AssertionDsc
    {
        optAssertionKind assertionKind;
        struct SsaVar
        {
            unsigned lclNum; // assigned to or property of this local var number
            unsigned ssaNum;
        };
        struct ArrBnd
        {
            ValueNum vnIdx;
            ValueNum vnLen;
        };
        struct AssertionDscOp1
        {
            optOp1Kind kind; // a normal LclVar, or Exact-type or Subtype
            ValueNum   vn;
            union {
                SsaVar lcl;
                ArrBnd bnd;
            };
        } op1;
        struct AssertionDscOp2
        {
            optOp2Kind kind; // a const or copy assignment
            ValueNum   vn;
            struct IntVal
            {
                ssize_t iconVal; // integer
#if !defined(HOST_64BIT)
                unsigned padding; // unused; ensures iconFlags does not overlap lconVal
#endif
                GenTreeFlags iconFlags; // gtFlags
            };
            union {
                SsaVar        lcl;
                IntVal        u1;
                __int64       lconVal;
                double        dconVal;
                IntegralRange u2;
            };
        } op2;

Memory layout:

1>class Compiler::AssertionDsc::AssertionDscOp1	size(16):
1>	+---
1> 0	| optOp1Kind kind
1> 4	| vn
1> 8	| SsaVar lcl
1> 8	| ArrBnd bnd
1>	+---


1>class Compiler::AssertionDsc::AssertionDscOp2	size(24):
1>	+---
1> 0	| optOp2Kind kind
1> 4	| vn
1> 8	| SsaVar lcl
1> 8	| IntVal u1
1> 8	| lconVal
1> 8	| dconVal
1> 8	| IntegralRange u2
1>	+---


1>class Compiler::AssertionDsc	size(48):
1>	+---
1> 0	| optAssertionKind assertionKind
1> 4	| AssertionDscOp1 op1
1>  	| <alignment member> (size=4)
1>24	| AssertionDscOp2 op2
1>	+---

Obvious things:

  1. we don't need that much space to store iconFlags (currently, we waste 8 bytes on it while there are only 22 possible values and they can't be mixed. - 1 byte is enough (5 bits actually)
  2. alignment member (x64 only?)

@EgorBo EgorBo assigned EgorBo and unassigned SingleAccretion Nov 21, 2022
@EgorBo EgorBo modified the milestones: Future, 8.0.0 Nov 21, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Nov 21, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Nov 23, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Dec 23, 2022
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 a pull request may close this issue.

2 participants