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

Assertion failed '!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum())' #57919

Closed
kunalspathak opened this issue Aug 23, 2021 · 7 comments · Fixed by #64804
Assignees
Labels
arch-x86 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI os-windows
Milestone

Comments

@kunalspathak
Copy link
Member

// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
//
// This file is auto-generated.
// Seed: -1
//

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
public class TestClass20045 {
    public struct S2 {
        public ulong uint64_1;
    }
    public struct S3 {
    }
    public struct S4 {
        public ulong uint64_2;
        public S2 s2_4;
    }
    public struct S5 {
    }
    static bool s_boolean_0 = true;
    static S2 s_s2_16 = new S2();
    static S3 s_s3_17 = new S3();
    public S3 Method1(out S5 p_s5_0, ref S2 p_s2_1) {
        unchecked {
            S4 s4_4 = new S4();
            do {
                try {
                    try {
                        try {
                            s4_4.uint64_2 = s4_4.s2_4.uint64_1;
                            {
                            }
                        } catch (TypeAccessException) {
                        } finally {
                        }
                    } catch (RankException) {
                    } catch (NotImplementedException) {
                    } finally {
                    }
                } catch (InvalidCastException) {
                } finally {
                }
            }
            while (15 > 4 || (s_boolean_0 = 15 <= 4));
            return s_s3_17;
        }
    }
    public void Method0() {
        unchecked {
            S5 s5_3 = new S5();
            s_s3_17 = Method1(out s5_3, ref s_s2_16);
            return;
        }
    }
    public static void Main(string[] args) {
        TestClass20045 objTestClass20045 = new TestClass20045();
        objTestClass20045.Method0();
    }
}
/*
Got output diff:
--------- Baseline ---------  

Environment:



--------- Test ---------  

Environment:



Assert failure(PID 12228 [0x00002fc4], Thread: 7360 [0x1cc0]): Assertion failed '!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum())' in 'TestClass20045:Method1(byref,byref):S3:this' during 'Lowering nodeinfo' (IL size 45)

    File: D:\git\runtime\src\coreclr\jit\lir.cpp Line: 1369
    Image: D:\git\runtime\artifacts\tests\coreclr\windows.x86.Checked\tests\Core_Root\CoreRun.exe




*/
@kunalspathak kunalspathak added arch-x86 os-windows area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI labels Aug 23, 2021
@dotnet-issue-labeler dotnet-issue-labeler bot added the untriaged New issue has not been triaged by the area owner label Aug 23, 2021
@ghost
Copy link

ghost commented Aug 23, 2021

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

Issue Details
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.
// See the LICENSE file in the project root for more information.
//
// This file is auto-generated.
// Seed: -1
//

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
public class TestClass20045 {
    public struct S2 {
        public ulong uint64_1;
    }
    public struct S3 {
    }
    public struct S4 {
        public ulong uint64_2;
        public S2 s2_4;
    }
    public struct S5 {
    }
    static bool s_boolean_0 = true;
    static S2 s_s2_16 = new S2();
    static S3 s_s3_17 = new S3();
    public S3 Method1(out S5 p_s5_0, ref S2 p_s2_1) {
        unchecked {
            S4 s4_4 = new S4();
            do {
                try {
                    try {
                        try {
                            s4_4.uint64_2 = s4_4.s2_4.uint64_1;
                            {
                            }
                        } catch (TypeAccessException) {
                        } finally {
                        }
                    } catch (RankException) {
                    } catch (NotImplementedException) {
                    } finally {
                    }
                } catch (InvalidCastException) {
                } finally {
                }
            }
            while (15 > 4 || (s_boolean_0 = 15 <= 4));
            return s_s3_17;
        }
    }
    public void Method0() {
        unchecked {
            S5 s5_3 = new S5();
            s_s3_17 = Method1(out s5_3, ref s_s2_16);
            return;
        }
    }
    public static void Main(string[] args) {
        TestClass20045 objTestClass20045 = new TestClass20045();
        objTestClass20045.Method0();
    }
}
/*
Got output diff:
--------- Baseline ---------  

Environment:



--------- Test ---------  

Environment:



Assert failure(PID 12228 [0x00002fc4], Thread: 7360 [0x1cc0]): Assertion failed '!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum())' in 'TestClass20045:Method1(byref,byref):S3:this' during 'Lowering nodeinfo' (IL size 45)

    File: D:\git\runtime\src\coreclr\jit\lir.cpp Line: 1369
    Image: D:\git\runtime\artifacts\tests\coreclr\windows.x86.Checked\tests\Core_Root\CoreRun.exe




*/
Author: kunalspathak
Assignees: -
Labels:

arch-x86, os-windows, area-CodeGen-coreclr

Milestone: -

@kunalspathak
Copy link
Member Author

Repros on windows/x86

@dotnet/jit-contrib

@JulieLeeMSFT JulieLeeMSFT removed the untriaged New issue has not been triaged by the area owner label Aug 23, 2021
@JulieLeeMSFT JulieLeeMSFT added this to the 6.0.0 milestone Aug 23, 2021
@BruceForstall
Copy link
Member

BruceForstall commented Sep 2, 2021

Simpler repro case:

using System;
using System.Runtime.CompilerServices;

public class TestClass20045 {
    public struct S2 {
        public ulong uint64_2;
    }
    public struct S4 {
        public ulong uint64_1;
        public S2 s2;
    }
    [MethodImpl(MethodImplOptions.NoInlining)]
    public static ulong Method1(bool b) {
        S4 s = new S4();
        do {
            s.uint64_1 = s.s2.uint64_2;
        } while (b);
        return s.s2.uint64_2;
    }
    public static void Main(string[] args) {
        ulong u = Method1(false);
        Console.WriteLine(u);
    }
}

@BruceForstall
Copy link
Member

This also fails in 5.0. Moving out to 7.0.

@BruceForstall BruceForstall modified the milestones: 6.0.0, 7.0.0 Sep 3, 2021
@AndyAyersMS
Copy link
Member

Do we do something reasonable in release jits?

@BruceForstall
Copy link
Member

Do we do something reasonable in release jits?

Yes, at least for this case.

LIR is trying to assert that if you have a local var store, there aren't any outstanding reads of that var that are "contained" and thus won't generate code until the "user" of the read. E.g.,

  t1 = LCL_VAR V01
  t2 = LCL_VAR V01
  STORE_LCL_VAR V01 <- t7 // here we would trash the "t2 = ..." which doesn't actually generate code if it is contained in the STORE_LCL_VAR below
  STORE_LCL_VAR V09 <- t2

Specifically, the comment says:

// Specifically, ensure that an unaliasable lclVar is not redefined between the
// point at which a use appears in linear order and the point at which it is used by its user.
// This ensures that it is always safe to treat a lclVar use as happening at the user (rather than at
// the lclVar node).

In this case, it's not an issue because we don't have a LCL_VAR; we have non-overlapping LCL_FLD. But CheckLclVarSemanticsHelper doesn't track LCL_FLD ranges.

The long decomposition looks like:

N001 (  3,  4) [000010] ------------        t10 =    LCL_FLD   long   V01 loc0         u:3[+8] Fseq[s2, uint64_2] $200
                                                  /--*  t10    long
N003 ( 10, 12) [000012] UA--G-------              *  STORE_LCL_FLD long   V01 loc0         ud:3->0[+0] Fseq[uint64_1]

=>

N001 (  3,  4) [000010] ------------        t10 =    LCL_FLD   int    V01 loc0         u:3[+8] Fseq[s2, uint64_2] $200
               [000043] ------------        t43 =    LCL_FLD   int    V01 loc0         [+12]
                                                  /--*  t10    int
                                                  +--*  t43    int
               [000044] ------------        t44 = *  LONG      long
                                                  /--*  t44    long
N003 ( 10, 12) [000012] UA--G-------              *  STORE_LCL_FLD long   V01 loc0         ud:3->0[+0] Fseq[uint64_1]

=> 

N001 (  3,  4) [000010] ------------        t10 =    LCL_FLD   int    V01 loc0         u:3[+8] Fseq[s2, uint64_2] $200
               [000043] ------------        t43 =    LCL_FLD   int    V01 loc0         [+12]
                                                  /--*  t10    int
N003 ( 10, 12) [000012] UA--G-------              *  STORE_LCL_FLD int    V01 loc0         ud:3->0[+0] Fseq[uint64_1]
                                                  /--*  t43    int
               [000045] U-----------              *  STORE_LCL_FLD int    V01 loc0         [+4]

In this case, the first STORE_LCL_FLD writing V01 doesn't trash the non-overlapping t43, reading V01 at a different offset/size, used by the second STORE_LCL_FLD.

It seems like the LIR checking should consider all non-overlapping LCL_FLD reads/writes as independent, like each was a LCL_VAR. But that would be a huge change.

We could simply (temporarily?) ignore this assert for LCL_FLD.

We could possibly change decomposition to try to avoid creating this form; maybe by creating copies through temps, which would presumably create worse CQ.

@jakobbotsch
Copy link
Member

Another repro, in debug mode/with tiering enabled:

// Generated by Fuzzlyn v1.5 on 2021-10-19 13:43:00
// Run on .NET 6.0.0-rc.2.21480.5 on X64 Windows
// Seed: 248268849421871353
// Reduced from 18.2 KiB to 0.2 KiB in 00:03:35
// Exits with error:
//
// Assert failure(PID 8056 [0x00001f78], Thread: 30444 [0x76ec]): Assertion failed '!nodeInfo.IsLclVarWrite() || !unusedLclVarReads.Contains(nodeInfo.LclNum())' in 'Program:Main(Fuzzlyn.ExecutionServer.IRuntime)' during 'Lowering nodeinfo' (IL size 30)
//
//     File: C:\dev\dotnet\runtime2\src\coreclr\jit\lir.cpp Line: 1367
//     Image: C:\dev\dotnet\runtime2\artifacts\tests\coreclr\windows.x86.Checked\Tests\Core_Root\corerun.exe
//
//
public struct S0
{
    public long F2;
}

public struct S1
{
    public S0 F0;
}

public class Program
{
    public static void Main()
    {
        var vr3 = new S1();
        var vr6 = vr3.F0;
        vr6.F2 = vr6.F2;
    }
}

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Feb 4, 2022
For LIR we verify that we can really consider locals to be used at their
user by having a checker that looks for interfering stores to the same
locals. However, in some cases we may have "interfering"
GT_LCL_FLD/GT_STORE_LCL_FLD, in the sense that they work on the same
local but on a disjoint range of bytes. Add support to validate this.

This fixes dotnet#57919 which made the fuzzer jobs very noisy and made it easy
to miss actual new examples (e.g. dotnet#63720 was just merged even though
there were real examples found there).

Fix dotnet#57919
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Feb 4, 2022
jakobbotsch added a commit that referenced this issue Feb 4, 2022
For LIR we verify that we can really consider locals to be used at their
user by having a checker that looks for interfering stores to the same
locals. However, in some cases we may have "interfering"
GT_LCL_FLD/GT_STORE_LCL_FLD, in the sense that they work on the same
local but on a disjoint range of bytes. Add support to validate this.

This fixes #57919 which made the fuzzer jobs very noisy and made it easy
to miss actual new examples (e.g. #63720 was just merged even though
there were real examples found there).

Fix #57919
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Feb 4, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Mar 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
arch-x86 area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI os-windows
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants