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

Fix fgValueNumberArrIndexVal for wide reads #58309

Merged
merged 4 commits into from
Aug 31, 2021
Merged

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Aug 28, 2021

Fixes #57914

Minimal repro:

private static bool Test1()
{
    byte[] array = new byte[2];
    array[0] = 1;
    array[1] = 2;


    byte  a1 = Unsafe.ReadUnaligned<byte>(ref array[0]);
    short a2 = Unsafe.ReadUnaligned<short>(ref array[0]);
    array[1] = 42;
    short a3 = Unsafe.ReadUnaligned<short>(ref array[0]);

    return a1 == a2 && a1 == a3; // returns True currently
}

a1, a2 and a3 all have different values here with the same liberal VN resulting Test1 to return true.

a1:

\--*  IND       byte   <l:$42, c:$401>
    \--*  ADD       byref  $300
        +--*  LCL_VAR   ref    V04 tmp1         u:2 $240
        \--*  CNS_INT   long   16 Fseq[#FirstElem] $181

a2:

\--*  IND       short  <l:$500, c:$5c1>
    \--*  LCL_VAR   byref  V07 tmp4         u:2 (last use) $300

($500 here is $42 with NullRefExc)

a3:

\--*  IND       byte   <l:$42, c:$402>
    \--*  ADD       byref  $300
        +--*  LCL_VAR   ref    V04 tmp1         u:2 (last use) $240
        \--*  CNS_INT   long   16 Fseq[#FirstElem] $181

The fix is quite conservative - it treats such array element loads as "non-proper" and assign a unique VN.
SuperPMI diffs are empty.

PS: Doesn't reproduce on .NET 5.0

/cc @dotnet/jit-contrib

@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 Aug 28, 2021
@ghost
Copy link

ghost commented Aug 28, 2021

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

Issue Details

Minimal repro:

private static bool Test1()
{
    byte[] array = new byte[2];
    array[0] = 1;
    array[1] = 2;


    byte  a1 = Unsafe.ReadUnaligned<byte>(ref array[0]);
    short a2 = Unsafe.ReadUnaligned<short>(ref array[0]);
    array[1] = 42;
    short a3 = Unsafe.ReadUnaligned<short>(ref array[0]);

    return a1 == a2 && a1 == a3;
}

a1, a2 and a3 all have different values here with the same liberal VN resulting Test1 to return true.

The fix is quite conservative - it treats such array element loads as "non-proper" and assign a unique VN.
Ideas for a less conservative fix are welcomed 🙂

PS: Doesn't reproduce on .NET 5.0

/cc @dotnet/jit-contrib

Author: EgorBo
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@SingleAccretion
Copy link
Contributor

SingleAccretion commented Aug 28, 2021

Heh, so I always wondered if it is possible to make VN use CASTs where it ought to use BITCASTs. Turns out it is very possible.

private static float Bitcast()
{
    int[] array = new int[2];
    array[0] = 1;
    
    return Unsafe.ReadUnaligned<float>(ref Unsafe.As<int, byte>(ref array[0]));
}
  wholeElem $41 is MapSelect(hAtArrTypeAtArr($340), ind=$142).
    VNForCastOper(float) is $42
    VNForCast($41, $42) returns $440 {FltCns[1.000000]}
  selectedElem is $440 after applying selectors.

But that is a separate issue (filed as #58312).

@EgorBo EgorBo added this to the 6.0.0 milestone Aug 28, 2021
Copy link
Contributor

@sandreenko sandreenko left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@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 Good,

@EgorBo EgorBo merged commit 098dc8b into dotnet:main Aug 31, 2021
@EgorBo
Copy link
Member Author

EgorBo commented Aug 31, 2021

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/runtime/actions/runs/1185769084

@ghost ghost locked as resolved and limited conversation to collaborators Sep 30, 2021
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.

[JitStressWithRandomNumbers] inliningVars.cmd fails
4 participants