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

Add support for TYP_BYREF LCL_FLDs to VN #64501

Merged
merged 3 commits into from Feb 6, 2022

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Jan 29, 2022

First, the code in fgAddFieldSeqForZeroOffset that was adding zero-offset sequence to LCL_FLD directly was incorrect, because fgAddFieldSeqForZeroOffset adds a sequence for the address that the passed-in node represents, while adding a field sequence to LCL_FLD directly means adding it to the "implicit" LCL_FLD_ADDR it indirects. One can reproduce this issue via code like the following (with JitNoStructPromotion=1):

private static int Problem(StructWithRawFldPtr p)
{
    return ((StructWithIndex*)p.RawFldPtr)->Index;
}

struct StructWithRawFldPtr
{
    public nint RawFldPtr;
}

struct StructWithIndex
{
    public int Index;
    public int Value;
}
LCL_FLD   long   V00 arg0         u:1[+0] Fseq[RawFldPtr, Index] (last use)

The first two commits fix this issue, in fgAddFieldSeqForZeroOffset and similar code in ChangeOper and we now get the lovely combination of LCL_FLD + zero-offset sequence in the map.

LCL_FLD   long   V00 arg0         u:1[+0] Fseq[RawFldPtr] (last use) Zero Fseq[Index]

That opens a possibility for another bug of course: partial sequences, that is cases where we will have ADD((LCL_FLD PtrToStatic [Zero FldSeq]), OFFSET FldSeq) and fail to incorporate the Zero FldSeq into the PtrToStatic's VN. This is fixed in the third commit.

Part of #58312.
Contributes to #63768.

No diffs as expected.

@msftbot msftbot bot added the community-contribution Indicates that the PR has been added by a community member label Jan 29, 2022
@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 Jan 29, 2022
@msftbot
Copy link
Contributor

msftbot bot commented Jan 29, 2022

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

Issue Details

First, the code in fgAddFieldSeqForZeroOffset that was adding zero-offset sequence to LCL_FLD directly was incorrect, because fgAddFieldSeqForZeroOffset adds a sequence for the address that the passed-in node represents, while adding a field sequence to LCL_FLD directly means adding it to the "implicit" LCL_FLD_ADDR it indirects. One can reproduce this issue via code like the following:

private static int Problem(StructWithRawFldPtr p)
{
    return ((StructWithIndex*)p.RawFldPtr)->Index;
}

struct StructWithRawFldPtr
{
    public nint RawFldPtr;
}

struct StructWithIndex
{
    public int Index;
    public int Value;
}
LCL_FLD   long   V00 arg0         u:1[+0] Fseq[RawFldPtr, Index] (last use)

The first two commits fix this issue, in fgAddFieldSeqForZeroOffset and similar code in ChangeOper and we now get the lovely combination of LCL_FLD + zero-offset sequence in the map.

LCL_FLD   long   V00 arg0         u:1[+0] Fseq[RawFldPtr] (last use) Zero Fseq[Index]

That opens a possibility for another bug of course: partial sequences, that is cases where we will have ADD((LCL_FLD PtrToStatic [Zero FldSeq]), OFFSET FldSeq) and fail to incorporate the Zero FldSeq into the PtrToStatic's VN. That is fixed in the third commit.

Part of #58312.
Contributes to #63768.

No diffs are expected.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr, community-contribution

Milestone: -

@SingleAccretion SingleAccretion marked this pull request as ready for review January 30, 2022 16:06
@SingleAccretion
Copy link
Contributor Author

Will assume the Win-Arm64 timeout is not related (it passed on the previous CI run).

@dotnet/jit-contrib

@SingleAccretion
Copy link
Contributor Author

Pushed some fixes for typos in commit messages (no code changes).

"fgAddFieldSeqForZeroOffset"'s contract is that it is passed
an *address*. If that address is a LCL_FLD, we must use the
"zero-offset sequence map", not the sequence of LCL_FLD itself,
as that represents LCL_FLD's own value, not the value it produces
(LCL_FLD == IND(LCL_FLD_ADDR), and LCL_FLD's sequence is the one
attached to LCL_FLD_ADDR, not IND).
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! Just to check my own understanding of the issue here, conceptually we can think of the zero-offset field seqs stored in the lookaside map to mean there is an implict ADD(x, 0) on top of the node with 0 having that field seq, but this code was mixing that up with the field accessed by x?

@SingleAccretion
Copy link
Contributor Author

Just to check my own understanding of the issue here, conceptually we can think of the zero-offset field seqs stored in the lookaside map to mean there is an implict ADD(x, 0) on top of the node with 0 having that field seq, but this code was mixing that up with the field accessed by x?

Yep, exactly.

@jakobbotsch jakobbotsch merged commit aeb7b91 into dotnet:main Feb 6, 2022
@SingleAccretion SingleAccretion deleted the Fix-ZeroOffs-LclFld branch February 6, 2022 15:40
@msftbot msftbot bot locked as resolved and limited conversation to collaborators Mar 8, 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 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.

None yet

2 participants