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

JIT: fix invariant analysis for cloning #70126

Merged
merged 3 commits into from
Jun 3, 2022

Conversation

AndyAyersMS
Copy link
Member

Fix some cases where the JIT was not sufficiently careful in verifing that
operands in a loop were suitably invariant.

Closes #61040.

Fix some cases where the JIT was not sufficiently careful in verifing that
operands in a loop were suitably invariant.

Closes dotnet#61040.
@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 Jun 2, 2022
@ghost ghost assigned AndyAyersMS Jun 2, 2022
@ghost
Copy link

ghost commented Jun 2, 2022

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

Issue Details

Fix some cases where the JIT was not sufficiently careful in verifing that
operands in a loop were suitably invariant.

Closes #61040.

Author: AndyAyersMS
Assignees: -
Labels:

area-CodeGen-coreclr

Milestone: -

@AndyAyersMS
Copy link
Member Author

@BruceForstall PTAL
cc @dotnet/jit-contrib

This is intended to have fairly minimal diffs (currently around 150 methods). Diffs will show both size decreases (because we stop cloning in some places) and size increases (because we lose a cloning condition and so either fail to remove bounds checks or fail to prove the slow loop is unreachable).

Spot checking showed the diffs to be reasonable; not clear if any of them were actual bugs or just places we though we knew something that we didn't actually know.

Some of the analysis here can be sharpened (eg to address #70100, and to improve the use of bit vectors, and possibly to allow some address-exposed cases). I will work on that in a follow-on change.


if (array->OperIs(GT_LCL_VAR))
{
if (!optIsVarAssgLoop(loopInd, array->AsLclVarCommon()->GetLclNum()))
Copy link
Member

Choose a reason for hiding this comment

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

Do you want to add JITDUMP if this fails, like you did above? (Same for if it's not a GT_LCL_VAR?)

Comment on lines 939 to 944
// Not clear why we return false here. We know the loop exit test
// is comparing iterVar to something; we just don't know what.
//
// No real difference between this case and the cases above where
// we don't set some kind of limit flag.
//
Copy link
Member

Choose a reason for hiding this comment

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

I think the caller expects one of the LPFLG_*_LIMIT flags to be set if this function returns true; you are currently not doing that for, e.g., cases where GT_ARR_LENGTH fails.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we do proper filtering later but will change this to match what we used to do.

@@ -977,7 +1001,7 @@ unsigned Compiler::optIsLoopIncrTree(GenTree* incr)
// optComputeIterInfo: Check tree is loop increment of a lcl that is loop-invariant.
//
// Arguments:
// from, to - are blocks (beg, end) which are part of the loop.
// lnum - loop in question
Copy link
Member

Choose a reason for hiding this comment

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

You didn't change the actual function signature.

It's also a little weird to pass around the loop index when this function is called as part of populating the loop info, so the loop table for this loop isn't complete (I would generally assume that if given a loop index, all the info about the loop at that index was valid).

Copy link
Member Author

Choose a reason for hiding this comment

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

You didn't change the actual function signature.

Thanks. I wanted to but ran into issues.

It's also a little weird to pass around the loop index

We've already recognized and recorded the loop, now we're just figuring out what other properties it may have. So I don't see why passing around the loop index seems problematic; it will still describe a valid loop no matter what this analysis might discover.

I'm using the index like this because the loop-based invariance analysis is more efficient and powerful than the block based invariance analysis, so I want to make as many queries as possible use the loop based checks.

The only one that can't (currently) is this check that the iterVar not have any other modifications within the loop. I may work on handling this somehow so that we only do one scan over the loop body and not two.

Copy link
Member

Choose a reason for hiding this comment

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

We've already recognized and recorded the loop, now we're just figuring out what other properties it may have.

Makes sense.

{
// Parent var index is too large, assume the worst.
//
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this return true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oops. Yes.

return optIsSetAssgLoop(lnum, vs) != 0;
}
else
{
if (varDsc->lvIsStructField)
{
return false;
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't this return true?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ditto.

@ghost ghost added the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 2, 2022
Comment on lines 5736 to 5737
varRefKinds refs = varTypeIsGC(tree->TypeGet()) ? VR_IND_REF : VR_IND_SCL;
desc->ivaMaskInd = varRefKinds(desc->ivaMaskInd | refs);
Copy link
Contributor

@SingleAccretion SingleAccretion Jun 2, 2022

Choose a reason for hiding this comment

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

Would it be out of scope for this change to also fix cases where "treating LCL_FLD conservatively as indirect access" is not enough, and where we can see OBJ/BLK/IND-based stores to locals?

Test cases that expose the two issues
using System;
using System.Runtime.CompilerServices;

unsafe class LoopLocalInvariance
{
    private const int ArrLen = 10;

    public static int Main()
    {
        int[] arr = new int[ArrLen];

        try
        {
            ProblemWithBlkAsg(arr, ArrLen);
            return 101;
        }
        catch (IndexOutOfRangeException) { }

        try
        {
            ProblemWithLclFldAsg(arr, ArrLen);
            return 102;
        }
        catch (IndexOutOfRangeException) { }

        return 100;
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void ProblemWithBlkAsg(int[] arr, int idx)
    {
        for (int i = 0; i < ArrLen; i++)
        {
            Unsafe.InitBlock(&i, (byte)idx, 1);
            arr[i] = 0;
        }
    }

    [MethodImpl(MethodImplOptions.NoInlining)]
    private static void ProblemWithLclFldAsg(int[] arr, int idx)
    {
        for (int i = 0; i < ArrLen; i++)
        {
            *(byte*)&i = (byte)idx;
            arr[i] = 0;
        }
    }
}

Copy link
Member Author

Choose a reason for hiding this comment

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

I'll take a look, sure.

I have follow on changes where I want to enable more cloning (an in more cases) so would be good to fix all the issues we know of with the analysis.

@ghost ghost removed the needs-author-action An issue or pull request that requires more info or actions from the author. label Jun 2, 2022
@AndyAyersMS
Copy link
Member Author

Failure is #69474.

@AndyAyersMS AndyAyersMS merged commit 1385479 into dotnet:main Jun 3, 2022
@AndyAyersMS
Copy link
Member Author

Left off adding a one test case. Will tack it onto a subsequent PR.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 3, 2022
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.

Loop cloning misses definitions for fields of promoted structs
3 participants