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 'store->TypeGet() == src->TypeGet()' during 'Optimize Valnum CSEs' #91443

Closed
kunalspathak opened this issue Sep 1, 2023 · 4 comments · Fixed by #91587
Closed
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

// Found by Antigen

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;
using System.Numerics;
public class TestClass
{
    static Vector128<ulong> s_v128_ulong_28 = Vector128.CreateScalar((ulong)0);
    static Vector3 s_v3_42 = new Vector3(-4.9651165f, -2.1474836E+09f, 1.0375f);
    int int_54 = 1;
    Vector128<ulong> v128_ulong_69 = Vector128<ulong>.AllBitsSet;
    Vector3 v3_83 = new Vector3(-0.84615386f, 1.0566038f, -4.9693875f);
    static int s_loopInvariant = 9;
    public void Method0()
    {
        unchecked
        {
            try
            {
                if (Vector128.LessThanAny(v128_ulong_69 ^ s_v128_ulong_28| (v128_ulong_69 += v128_ulong_69), v128_ulong_69 & Vector128<ulong>.AllBitsSet& v128_ulong_69))
                {
                }
                else
                {
                    int __loopvar0 = s_loopInvariant;
                    while (15!=4)
                    {
                        if (__loopvar0 >= 15+4)
                            break;
                    }
                    switch (int_54)
                    {
                        case 0:
                        {
                            Vector3.Subtract(Vector3.Cross(s_v3_42, s_v3_42) + v3_83 - v3_83+ s_v3_42 * s_v3_42+ v3_83, s_v3_42 = v3_83 - s_v3_42- s_v3_42 - s_v3_42+ Vector3.Clamp(v3_83, v3_83, Vector3.UnitY));
                            break;
                        }
                        default:
                        {
                            break;
                        }
                    }
                }
            }
            finally
            {
            }
            return;
        }
    }
    public static void Main(string[] args)
    {
        new TestClass().Method0();
    }
}
/*
Environment:


Assert failure(PID 43728 [0x0000aad0], Thread: 32844 [0x804c]): Assertion failed 'store->TypeGet() == src->TypeGet()' in 'TestClass:Method0():this' during 'Optimize Valnum CSEs' (IL size 250; hash 0x46e9aa75; Tier0-FullOpts)
    File: D:\git\runtime\src\coreclr\jit\importer.cpp Line: 785
    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 Sep 1, 2023
@ghost
Copy link

ghost commented Sep 1, 2023

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

Issue Details
// Found by Antigen

using System;
using System.Collections.Generic;
using System.Runtime.CompilerServices;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;
using System.Numerics;
public class TestClass
{
    static Vector128<ulong> s_v128_ulong_28 = Vector128.CreateScalar((ulong)0);
    static Vector3 s_v3_42 = new Vector3(-4.9651165f, -2.1474836E+09f, 1.0375f);
    int int_54 = 1;
    Vector128<ulong> v128_ulong_69 = Vector128<ulong>.AllBitsSet;
    Vector3 v3_83 = new Vector3(-0.84615386f, 1.0566038f, -4.9693875f);
    static int s_loopInvariant = 9;
    public void Method0()
    {
        unchecked
        {
            try
            {
                if (Vector128.LessThanAny(v128_ulong_69 ^ s_v128_ulong_28| (v128_ulong_69 += v128_ulong_69), v128_ulong_69 & Vector128<ulong>.AllBitsSet& v128_ulong_69))
                {
                }
                else
                {
                    int __loopvar0 = s_loopInvariant;
                    while (15!=4)
                    {
                        if (__loopvar0 >= 15+4)
                            break;
                    }
                    switch (int_54)
                    {
                        case 0:
                        {
                            Vector3.Subtract(Vector3.Cross(s_v3_42, s_v3_42) + v3_83 - v3_83+ s_v3_42 * s_v3_42+ v3_83, s_v3_42 = v3_83 - s_v3_42- s_v3_42 - s_v3_42+ Vector3.Clamp(v3_83, v3_83, Vector3.UnitY));
                            break;
                        }
                        default:
                        {
                            break;
                        }
                    }
                }
            }
            finally
            {
            }
            return;
        }
    }
    public static void Main(string[] args)
    {
        new TestClass().Method0();
    }
}
/*
Environment:


Assert failure(PID 43728 [0x0000aad0], Thread: 32844 [0x804c]): Assertion failed 'store->TypeGet() == src->TypeGet()' in 'TestClass:Method0():this' during 'Optimize Valnum CSEs' (IL size 250; hash 0x46e9aa75; Tier0-FullOpts)
    File: D:\git\runtime\src\coreclr\jit\importer.cpp Line: 785
    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: -

@ghost ghost added the untriaged New issue has not been triaged by the area owner label Sep 1, 2023
@JulieLeeMSFT JulieLeeMSFT added this to the 8.0.0 milestone Sep 1, 2023
@ghost ghost removed the untriaged New issue has not been triaged by the area owner label Sep 1, 2023
@JulieLeeMSFT
Copy link
Member

@jakobbotsch, PTAL.

@EgorBo
Copy link
Member

EgorBo commented Sep 1, 2023

Reduced repro:

public class TestClass
{
    static Vector3 s;

    [MethodImpl(MethodImplOptions.NoInlining)]
    public void Method0()
    {
        Vector3.Cross(s, s);
    }

    public static void Main(string[] args)
    {
        new TestClass().Method0();
    }
}

Presumably the assert happens when we optimize IND<simd> into IND<byte> (nullcheck) under COMMA

@jakobbotsch
Copy link
Member

The real problem is that inlining (impStoreStruct more precisely) produces non-value but typed commas, e.g. it changes

               [000006] --CXG------COMMA     simd12
               [000005] H-CXG------                         ├──▌  CALL help int    CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000003] ----------- arg0                    │  ├──▌  CNS_INT   int    0x8A2B390
               [000004] ----------- arg1                    │  └──▌  CNS_INT   int    1
               [000001] I---G------                         └──▌  IND       simd12
               [000000] H----------                            └──▌  CNS_INT(h) int    0x8601620 static Fseq[s]

to

               [000006] -ACXG------COMMA     simd12
               [000005] H-CXG------                         ├──▌  CALL help int    CORINFO_HELP_GETSHARED_NONGCSTATIC_BASE
               [000003] ----------- arg0                    │  ├──▌  CNS_INT   int    0x8A2B390
               [000004] ----------- arg1                    │  └──▌  CNS_INT   int    1
               [000087] DA--G------                         └──▌  STORE_LCL_VAR simd12<System.Numerics.Vector3> V02 tmp1         
               [000001] I---G------                            └──▌  IND       simd12
               [000000] H----------                               └──▌  CNS_INT(h) int    0x8601620 static Fseq[s]

where we usually would expect [000006] to be TYP_VOID (which is e.g. what gtExtractSideEffList produces).

I've tried fixing this once but I ran into a bunch of issues (first one being that few places expect the "value" node passed to gtNewTempStore to be retyped). It's possible it is easier to fix today by not sinking stores below COMMAs anymore, which is probably not necessary anymore after #83590. But for .NET 8 I will try to see if there's something more surgical.

jakobbotsch added a commit to jakobbotsch/runtime that referenced this issue Sep 5, 2023
Morph has post-order logic to compensate for mistyped commas produced by
impStoreStruct. However, block morphing can optimize unused stores into
INDs; this interacts with the mistyped commas to produce illegal IR
shapes (e.g. `COMMA<simd12>(..., IND<ubyte>(...))`).

The ideal solution is to fix impStoreStruct (dotnet#91586 tracks this), but
this change has a more surgical fix for the problem that can be
backported to .NET 8.

Fix dotnet#91443
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Sep 5, 2023
jakobbotsch added a commit that referenced this issue Sep 7, 2023
Morph has post-order logic to compensate for mistyped commas produced by
impStoreStruct. However, block morphing can optimize unused stores into
INDs; this interacts with the mistyped commas to produce illegal IR
shapes (e.g. `COMMA<simd12>(..., IND<ubyte>(...))`).

The ideal solution is to fix impStoreStruct (#91586 tracks this), but
this change has a more surgical fix for the problem that can be
backported to .NET 8.

Fix #91443
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Sep 7, 2023
github-actions bot pushed a commit that referenced this issue Sep 7, 2023
Morph has post-order logic to compensate for mistyped commas produced by
impStoreStruct. However, block morphing can optimize unused stores into
INDs; this interacts with the mistyped commas to produce illegal IR
shapes (e.g. `COMMA<simd12>(..., IND<ubyte>(...))`).

The ideal solution is to fix impStoreStruct (#91586 tracks this), but
this change has a more surgical fix for the problem that can be
backported to .NET 8.

Fix #91443
jeffschwMSFT added a commit that referenced this issue Sep 11, 2023
…91718)

* JIT: Compensate for mistyped commas in morph pre-order too

Morph has post-order logic to compensate for mistyped commas produced by
impStoreStruct. However, block morphing can optimize unused stores into
INDs; this interacts with the mistyped commas to produce illegal IR
shapes (e.g. `COMMA<simd12>(..., IND<ubyte>(...))`).

The ideal solution is to fix impStoreStruct (#91586 tracks this), but
this change has a more surgical fix for the problem that can be
backported to .NET 8.

Fix #91443

* Fix build

---------

Co-authored-by: Jakob Botsch Nielsen <jakob.botsch.nielsen@gmail.com>
Co-authored-by: Jeff Schwartz <jeffschw@microsoft.com>
@ghost ghost locked as resolved and limited conversation to collaborators Oct 7, 2023
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.

4 participants