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 numbering of exposed LCL_VARs #79772

Merged
merged 4 commits into from
Jan 11, 2023

Conversation

SingleAccretion
Copy link
Contributor

@SingleAccretion SingleAccretion commented Dec 16, 2022

The conservative VN must be unique for different uses lest we risk running into memory safety issues, by, e. g., removing range checks based on "racy" data.

It is very easy to reproduce bad codegen with this simple method and JitNoCSE=1:

[MethodImpl(MethodImplOptions.NoInlining)]
private static int Problem(int[] a, int b)
{
    JitUse(&b);
    return a[b] + a[b];
}

Assertion propagation removes the second check, which is not legal as b's value may change once we get to it from the first check.

Diffs will be regressions; I see in some cases we did illegal bounds checks removal.

The conservative VN must be unique for different uses lest
we risk running into memory safety issues, by, e. g. removing
range checks based on "racy" data.
@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 Dec 16, 2022
@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Dec 16, 2022
@ghost
Copy link

ghost commented Dec 16, 2022

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

Issue Details

The conservative VN must be unique for different uses lest we risk running into memory safety issues, by, e. g., removing range checks based on "racy" data.

It is very easy to reproduce bad codegen with this simple method and JitNoCSE=1:

[MethodImpl(MethodImplOptions.NoInlining)]
private static void Problem(int[] a, int b)
{
    JitUse(&b);
    _ = a[b];
    _ = a[b];
}

Assertion propagation removes the second check, which is not legal as b's value may change once we get to it from the first check.

Author: SingleAccretion
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

Without the fix, 4 out of 5 runs fail on my machine.
@SingleAccretion SingleAccretion marked this pull request as ready for review December 17, 2022 12:58
@SingleAccretion
Copy link
Contributor Author

@dotnet/jit-contrib

@jakobbotsch
Copy link
Member

/azp run runtime-coreclr superpmi-asmdiffs

@azure-pipelines
Copy link

No pipelines are associated with this pull request.

@jakobbotsch
Copy link
Member

/azp run runtime, runtime-coreclr superpmi-diffs

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@jakobbotsch
Copy link
Member

Looks like the new test needs to be disabled on wasm.

@SingleAccretion
Copy link
Contributor Author

Looks like the test is failing on x64 under the interpreter as well. Will disable it there and create an issue once the FullAot tests are done.

@jakobbotsch jakobbotsch merged commit 0e17421 into dotnet:main Jan 11, 2023
@jakobbotsch
Copy link
Member

Thanks! Let's see if this shows up in perf triage...

@AndyAyersMS
Copy link
Member

FYI, I am seeing this new test crash on arm64 under PGO stress as part of #80481 -- suspect it's not related to that PR but haven't drilled in yet.

https://dev.azure.com/dnceng-public/public/_build/results?buildId=133883&view=ms.vss-test-web.build-test-results-tab

@SingleAccretion
Copy link
Contributor Author

Yes, in all likelihood not related. We've now seen it fail (not crash though) on x86 too. I will put up a change to disable the test for now.

@SingleAccretion SingleAccretion deleted the Fix-Addr-Exposed-VN-Bug branch February 5, 2023 18:55
@ghost ghost locked as resolved and limited conversation to collaborators Mar 7, 2023
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.

3 participants