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

Assign VN for COMMA in gtWrapWithSideEffects #108955

Merged
merged 10 commits into from
Oct 17, 2024

Conversation

EgorBo
Copy link
Member

@EgorBo EgorBo commented Oct 17, 2024

Let's see if this works out on CI..

@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 Oct 17, 2024
Copy link
Contributor

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

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2024

@MihuBot -dependsOn 108838

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2024

PTAL @AndyAyersMS @dotnet/jit-contrib
I expected more diffs, but there are just a few. The main motivation was to optimize bound checks for FOH-allocated arrays, repro:

using System;
using System.Runtime.CompilerServices;

class Program
{
    static void Main()
    {
        for (int i=0; i<200; i++) {
            Test();
            Thread.Sleep(16);
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    static int Test() => MyArray.arr[2]; // should drop bound checks, array length is known
}

class MyArray {
    public static readonly int[] arr = new int[10];
}

at least, it's a clean up..

Comment on lines 17226 to 17228
GenTree* comma = gtNewOperNode(GT_COMMA, tree->TypeGet(), sideEffects, tree);
comma->gtVNPair = tree->gtVNPair;
return comma;
Copy link
Contributor

Choose a reason for hiding this comment

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

This needs to attach the exception set from sideEffects.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks! Addressed

@MichalPetryka
Copy link
Contributor

@MihuBot -dependsOn 108838,108606

@MichalPetryka
Copy link
Contributor

I expected more diffs, but there are just a few.

See above for more diffs with #108606

@EgorBo
Copy link
Member Author

EgorBo commented Oct 17, 2024

SPMI failures are due to JIT-EE bump in a PR merged recently

@EgorBo EgorBo merged commit 07e9313 into dotnet:main Oct 17, 2024
103 of 108 checks passed
@EgorBo EgorBo deleted the fix-vn-after-constprop branch October 17, 2024 16:13
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.

4 participants