Skip to content

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.
@ghost ghost added 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 labels 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