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

Proper VNs for zeroed structs #61285

Merged

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Nov 6, 2021

Previously, zero-initialized struct locals were given a special $VNForZeroMap, which had the nice property that VNForMapSelect($VNForZeroMap, Field) would always return zero of the appropriate type. However, the fact that this VN is not unique meant that it was dangerous to propagate it, e. g. through copies, which was hidden by the fact that ApplySelectorsTypeCheck's logic is very conservative.

This change introduces a new VNFunc to represent zeroed objects - VNF_ZeroObj, that takes a struct handle as its argument and is thus free of the aforementioned problem and can be propagated freely and not treated specially. It also, of course, retains the ZeroMap's selection property.

There are almost no diffs (win-x64, win-x86, win-arm64) for this change because of some other deficiencies in assertion and copy propagation, they are being/will be fixed separately, so the main point of this change is to contribute to #58312.

Notes about the diffs

Some are exactly what we'd expect: dead code elimination due to better propagation of zeroes though struct copies:

Struct a = default;

{ ... other blocks ... }

Struct b = a;

{ ... other blocks ... }

if (b.Field == 0) { ... } // Now we can recognize this as always true

Others are bizarre: CSEs created for the IND trees like the following:

N011 ( 26, 17)              [000045] --CXG-------                 +--*  CALL      int    B.Test_Nested $14b
N010 ( 12, 14)              [000174] -c--G------- arg0 x0,x1      |  \--*  FIELD_LIST struct $8b
N004 (  6,  7) CSE #01 (use)[000175] n---G------- ofs 0           |     +--*  IND       long   $200
N003 (  3,  5)              [000046] ----G-------                 |     |  \--*  ADDR      byref  $1c6
N002 (  3,  4)              [000040] -------N----                 |     |     \--*  LCL_FLD   struct V00 loc0         u:2[+144] Fseq[hackishFieldName, hackishFieldName] $84
N009 (  6,  7)              [000180] n---G------- ofs 8           |     \--*  IND       int    $14a
N008 (  5,  8)              [000179] ----G--N----                 |        \--*  ADD       byref  $1c7
N006 (  3,  5)              [000176] ----G-------                 |           +--*  ADDR      byref  $1c6
N005 (  3,  4)              [000177] -------N----                 |           |  \--*  LCL_FLD   struct V00 loc0         u:2[+144] Fseq[hackishFieldName, hackishFieldName] (last use) $84
N007 (  1,  2)              [000178] ------------                 |           \--*  CNS_INT   long   8 $240

Here we have another case of type mismatch: VN thinks that the fields in the list are in fact struct fields:

***** BB07, STMT00005(before)
N014 ( 30, 22) [000050] --CXG-------              *  JTRUE     void  
N013 ( 28, 20) [000049] J-CXG--N----              \--*  EQ        int   
N011 ( 26, 17) [000045] --CXG-------                 +--*  CALL      int    B.Test_Nested
N010 ( 12, 14) [000174] -c--G------- arg0 x0,x1      |  \--*  FIELD_LIST struct
N004 (  6,  7) [000175] n---G------- ofs 0           |     +--*  IND       long  
N003 (  3,  5) [000046] ----G-------                 |     |  \--*  ADDR      byref 
N002 (  3,  4) [000040] -------N----                 |     |     \--*  LCL_FLD   struct V00 loc0         u:2[+144] Fseq[hackishFieldName, hackishFieldName]
N009 (  6,  7) [000180] n---G------- ofs 8           |     \--*  IND       int   
N008 (  5,  8) [000179] ----G--N----                 |        \--*  ADD       byref 
N006 (  3,  5) [000176] ----G-------                 |           +--*  ADDR      byref 
N005 (  3,  4) [000177] -------N----                 |           |  \--*  LCL_FLD   struct V00 loc0         u:2[+144] Fseq[hackishFieldName, hackishFieldName] (last use)
N007 (  1,  2) [000178] ------------                 |           \--*  CNS_INT   long   8
N012 (  1,  2) [000048] ------------                 \--*  CNS_INT   int    100

N001 [000173]   ARGPLACE  => $8a {MemOpaque:NotInLoop}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $46, fieldType is struct, size = 48
    VNForMapSelect($80, $46):struct returns $83 {ZeroObj($42)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $45, fieldType is struct, size = 12
    VNForMapSelect($83, $45):struct returns $84 {ZeroObj($44)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $46, fieldType is struct, size = 48
    VNForMapSelect($80, $46):struct returns $83 {ZeroObj($42)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $45, fieldType is struct, size = 12
    VNForMapSelect($83, $45):struct returns $84 {ZeroObj($44)}
N002 [000040]   LCL_FLD   V00 loc0         u:2[+144] Fseq[hackishFieldName, hackishFieldName] => $84 {ZeroObj($44)}
    FieldSeq {hackishFieldName} is $182
    FieldSeq {(hackishFieldName, hackishFieldName)} is $185
N003 [000046]   ADDR      => $1c6 {PtrToLoc($c0, $185)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $46, fieldType is struct, size = 48
    VNForMapSelect($80, $46):struct returns $83 {ZeroObj($42)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $45, fieldType is struct, size = 12
    VNForMapSelect($83, $45):struct returns $84 {ZeroObj($44)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $46, fieldType is struct, size = 48
    VNForMapSelect($80, $46):struct returns $83 {ZeroObj($42)}
  VNApplySelectors:
    VNForHandle(hackishFieldName) is $45, fieldType is struct, size = 12
    VNForMapSelect($83, $45):struct returns $84 {ZeroObj($44)}
N004 [000175]   IND       => $200 {$84, long <- struct}

This is "ok" because it only thinks that for the first field (subsequent ones get PtrToLoc with NotAField VNs because the constant offsets do not have field sequences), and the long type of this "field" is fixed by the ABI, so this VN represents a properly unique value. It is definitely not by design though.

Previously such INDs got $VN.ZeroMap assigned to them (which is only supposed to be used for structs!) and did not get CSEd.

Reproduction for this:

private static int Problem(LargeStruct valueArg, int c)
{
    var value = valueArg;

    if (c is 1)
    {
        return 1;
    }

    CallForMultiRegStruct(value.SmallerStruct1.MultiRegStruct1);

    return 0;
}

[MethodImpl(MethodImplOptions.NoInlining)]
static void CallForMultiRegStruct(MultiRegStruct value) { }

struct MultiRegStruct
{
    public long FirstValue;
    public long SecondValue;
}

struct LargeStruct
{
    public SmallerStruct SmallerStruct1;
    public SmallerStruct SmallerStruct2;
}

struct SmallerStruct
{
    public MultiRegStruct MultiRegStruct1;
    public MultiRegStruct MultiRegStruct2;
    public MultiRegStruct MultiRegStruct3;
}

Part of #58312.

@dotnet-issue-labeler dotnet-issue-labeler bot added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Nov 6, 2021
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 6, 2021
@ghost
Copy link

ghost commented Nov 6, 2021

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

Issue Details

Previously, zero-initialized struct locals were given a special $VNForZeroMap, which had the nice property that VNForMapSelect($VNForZeroMap, Field) would always return zero of the appropriate type. However, the fact that this VN is not unique meant that it was dangerous to propagate it, e. g. through copies, which was hidden by the fact that ApplySelectorsTypeCheck's logic is very conservative.

This change introduces a new VNFunc to represent zeroed objects - VNF_ZeroObj, that takes a struct handle as its argument and is thus free of the aforementioned problem and can be propagated freely and not treated specially. It also, of course, retains the ZeroMap's selection property.

There are almost no diffs for this change because of some other deficiencies in assertion and copy propagation, they are being/will be fixed separately, so the main point of this change is to contribute to #58312.

Part of #58312.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion changed the title Proper VN for zeroed structs Proper VNs for zeroed structs Nov 6, 2021
@SingleAccretion SingleAccretion force-pushed the Proper-VN-For-Zeroed-Structs branch 5 times, most recently from a6f1b90 to 0d5a6c0 Compare November 7, 2021 22:32
Previously, zero-initialized struct locals were given
a special $VNForZeroMap, which had the nice property
that VNForMapSelect($VNForZeroMap, Field) would always
return zero of the appropriate type. However, the fact
that this VN is not unique meant that it was dangerous
to propagate it, e. g. through copies, which was hidden
by the fact that ApplySelectorsTypeCheck's logic is very
conservative.

This change introduces a new VNFunc to represent zeroed
objects - VNF_ZeroObj, that takes a struct handle as its
argument and is thus free of the aforementioned problem and
can be propagated freely and not treated specially. It
also, of course, retains the ZeroMap's selection property.

There are almost no diffs for this change because of some
other deficiencies in assertion and copy propagation, they
are being/will be fixed separately.
Previously this codepath was reachable, but did not matter as
the returned values were immediately discarded because of the
conservative logic in VNApplySelectorsTypeCheck. This is still
more or less the case, but let's just fix it properly.
@SingleAccretion SingleAccretion marked this pull request as ready for review November 8, 2021 14:18
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch jakobbotsch self-assigned this Nov 8, 2021
@jakobbotsch jakobbotsch added this to the 7.0.0 milestone Nov 8, 2021
@jakobbotsch jakobbotsch self-requested a review November 8, 2021 16:13
Copy link
Member

@jakobbotsch jakobbotsch left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

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 community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants