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

[release/5.0-rc2] Fix for folding of *(typ*)&lclVar for small types #41838

Merged

Conversation

echesakov
Copy link
Contributor

Backport of #40871 to release/5.0-rc2

Customer Impact

The underlying issue was discovered during one of libraries-jitstress test runs after #40535 was merged. After thorough analysis I could reproduce the issue without any stress mode by simple marking one of the function in libraries with AggressiveInlining attribute.

Later, I constructed a simple C# test case (see src/tests/JIT/Regression/JitBlue/Runtime_40607/Runtime_40607.cs)

        [MethodImpl(MethodImplOptions.AggressiveInlining)]
        static bool WillBeInlined(out bool shouldBeFalse)
        {
            shouldBeFalse = false;
            return true;
        }

        [MethodImpl(MethodImplOptions.NoInlining)]
        [SkipLocalsInit]
        static int DependsOnUnInitValue()
        {
            int retVal = 1;
            bool shouldBeFalse;

            while (WillBeInlined(out shouldBeFalse))
            {
                if (shouldBeFalse)
                {
                    retVal = 0;
                }
                break;
            }

            return retVal;
        }

Here the result of DependsOnUnInitValue depends on uninitialized value of stack local variable shouldBeFalse, even though it's clear from the program that it should always has false value.

Such issue can be reproduced when first loading of such local would be mophed earlier than its first store. The test case mentioned above is generalized for other small types (see src/tests/JIT/Regression/JitBlue/Runtime_40607/Runtime_40607.il)
This PR fixes an issue in a slightly different way than #40535 and, as mentioned in #40871 (comment), remedies most of performance regression introduced by #40253

Testing

I completed jitstress test runs with coreclr and libraries tests.

Risk

Low to moderate.

@dotnet/jit-contrib

1) it is *definitely load* and types of both indirection and local variable have the same signedness (e.g. bool and byte)
2) otherwise, fold the tree and mark the local node with GTF_VAR_FOLDED_IND
   and call fgDoNormalizeOnStore() on such nodes' parents in post-order morph.
@echesakov echesakov added the area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI label Sep 3, 2020
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,
Note: this PR fixes a silent bad codegen regression introduced in 5.0.

Copy link
Member

@jeffschwMSFT jeffschwMSFT left a comment

Choose a reason for hiding this comment

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

Approved. I will take it for consideration next week.

@jeffschwMSFT jeffschwMSFT added the Servicing-consider Issue for next servicing release review label Sep 4, 2020
@echesakov
Copy link
Contributor Author

echesakov commented Sep 5, 2020

Note that this PR will also close performance regression #41677 introduced by #40535
As described in #41677 (comment)

@echesakov echesakov linked an issue Sep 5, 2020 that may be closed by this pull request
@danmoseley danmoseley added Servicing-approved Approved for servicing release and removed Servicing-consider Issue for next servicing release review labels Sep 8, 2020
@echesakov echesakov merged commit 343a0b6 into dotnet:release/5.0-rc2 Sep 8, 2020
@echesakov echesakov deleted the Release_5.0-rc2-Runtime_40607 branch September 8, 2020 17:57
@ghost ghost locked as resolved and limited conversation to collaborators Dec 7, 2020
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 Servicing-approved Approved for servicing release
Projects
None yet
4 participants