Assertion failed '!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER)' during 'Optimize index checks'

[kunalspathak]

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
{
    public struct S1
    {
        public struct S1_D1_F3
        {
        }
    }
    public struct S2
    {
    }
    static byte s_byte_5 = 127;
    static Vector128<ushort> s_v128_ushort_21 = Vector128.Create((ushort)2, 2, 5, 32767, 2, 5, 54, 5);
    static Vector128<int> s_v128_int_22 = Vector128<int>.Zero;
    static S1.S1_D1_F3 s_s1_s1_d1_f3_51 = new S1.S1_D1_F3();
    static S1 s_s1_52 = new S1();
    static S2 s_s2_53 = new S2();
    S1.S1_D1_F3 s1_s1_d1_f3_101 = new S1.S1_D1_F3();
    S1 s1_102 = new S1();
    S2 s2_103 = new S2();
    public S1 Method54(out S1 p_s1_1625, out S1.S1_D1_F3 p_s1_s1_d1_f3_1626, ref S1.S1_D1_F3 p_s1_s1_d1_f3_1627, S1 p_s1_1628, S1 p_s1_1629, ref S1 p_s1_1630, ref S1.S1_D1_F3 p_s1_s1_d1_f3_1631, out S2 p_s2_1632, out S2 p_s2_1633)
    {
        unchecked
        {
            byte byte_1635 = 2;
            int int_1640 = 2;
            ushort ushort_1645 = 54;
            if (15<=4)
            {
            }
            else
            {
                ushort_1645 += Vector128.GetElement(Sse2.ShiftRightLogical128BitLane(s_v128_ushort_21 = s_v128_ushort_21, s_byte_5 = s_byte_5), Sse41.Extract(s_v128_int_22, byte_1635) + (int_1640 *= 15-4)& 5);
            }
            return s_s1_52;
        }
    }
    public void Method0()
    {
        unchecked
        {
            S1.S1_D1_F3 s1_s1_d1_f3_2935 = new S1.S1_D1_F3();
            S1 s1_2936 = new S1();
            s1_2936 = Method54(out s_s1_52, out s1_s1_d1_f3_2935, ref s1_s1_d1_f3_101, s_s1_52, s_s1_52, ref s1_102, ref s_s1_s1_d1_f3_51, out s_s2_53, out s2_103);
            return;
        }
    }
    public static void Main(string[] args)
    {
        new TestClass().Method0();
    }
}
/*
Environment:

set DOTNET_TieredCompilation=0

Assert failure(PID 27016 [0x00006988], Thread: 45140 [0xb054]): Assertion failed '!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER)' in 'TestClass:Method54(byref,byref,byref,TestClass+S1,TestClass+S1,byref,byref,byref,byref):TestClass+S1:this' during 'Optimize index checks' (IL size 69; hash 0x879627a5; FullOpts)
    File: D:\git\runtime\src\coreclr\jit\gentree.cpp Line: 16783
    Image: D:\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\tests\Core_Root\CoreRun.exe
*/

[ghost]

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

Issue Details

---

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
{
    public struct S1
    {
        public struct S1_D1_F3
        {
        }
    }
    public struct S2
    {
    }
    static byte s_byte_5 = 127;
    static Vector128<ushort> s_v128_ushort_21 = Vector128.Create((ushort)2, 2, 5, 32767, 2, 5, 54, 5);
    static Vector128<int> s_v128_int_22 = Vector128<int>.Zero;
    static S1.S1_D1_F3 s_s1_s1_d1_f3_51 = new S1.S1_D1_F3();
    static S1 s_s1_52 = new S1();
    static S2 s_s2_53 = new S2();
    S1.S1_D1_F3 s1_s1_d1_f3_101 = new S1.S1_D1_F3();
    S1 s1_102 = new S1();
    S2 s2_103 = new S2();
    public S1 Method54(out S1 p_s1_1625, out S1.S1_D1_F3 p_s1_s1_d1_f3_1626, ref S1.S1_D1_F3 p_s1_s1_d1_f3_1627, S1 p_s1_1628, S1 p_s1_1629, ref S1 p_s1_1630, ref S1.S1_D1_F3 p_s1_s1_d1_f3_1631, out S2 p_s2_1632, out S2 p_s2_1633)
    {
        unchecked
        {
            byte byte_1635 = 2;
            int int_1640 = 2;
            ushort ushort_1645 = 54;
            if (15<=4)
            {
            }
            else
            {
                ushort_1645 += Vector128.GetElement(Sse2.ShiftRightLogical128BitLane(s_v128_ushort_21 = s_v128_ushort_21, s_byte_5 = s_byte_5), Sse41.Extract(s_v128_int_22, byte_1635) + (int_1640 *= 15-4)& 5);
            }
            return s_s1_52;
        }
    }
    public void Method0()
    {
        unchecked
        {
            S1.S1_D1_F3 s1_s1_d1_f3_2935 = new S1.S1_D1_F3();
            S1 s1_2936 = new S1();
            s1_2936 = Method54(out s_s1_52, out s1_s1_d1_f3_2935, ref s1_s1_d1_f3_101, s_s1_52, s_s1_52, ref s1_102, ref s_s1_s1_d1_f3_51, out s_s2_53, out s2_103);
            return;
        }
    }
    public static void Main(string[] args)
    {
        new TestClass().Method0();
    }
}
/*
Environment:

set DOTNET_TieredCompilation=0

Assert failure(PID 27016 [0x00006988], Thread: 45140 [0xb054]): Assertion failed '!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER)' in 'TestClass:Method54(byref,byref,byref,TestClass+S1,TestClass+S1,byref,byref,byref,byref):TestClass+S1:this' during 'Optimize index checks' (IL size 69; hash 0x879627a5; FullOpts)
    File: D:\git\runtime\src\coreclr\jit\gentree.cpp Line: 16783
    Image: D:\git\runtime\artifacts\tests\coreclr\windows.x64.Checked\tests\Core_Root\CoreRun.exe
*/

Author:	kunalspathak
Assignees:	-
Labels:	os-windows, arch-x64, area-CodeGen-coreclr
Milestone:	8.0.0

[kunalspathak]

We tend to extract the side-effect for [000064] for the flag GT_ASG:

N012 ( 39, 38) [000045] -ACXG------                         ▌  BOUNDS_CHECK_ArgRng void   $3c2
N010 ( 34, 30) [000040] -ACXG------                         ├──▌  AND       int    $345
N008 ( 32, 28) [000038] -ACXG------                         │  ├──▌  ADD       int    $344
N006 ( 30, 26) [000064] -ACXG------                         │  │  ├──▌  CALL      int    System.Runtime.Intrinsics.X86.Sse41:Extract(System.Runtime.Intrinsics.Vector128`1[int],ubyte):int $c1
N003 (  9, 15) [000070] DA--G------ arg0 setup              │  │  │  ├──▌  STORE_LCL_VAR simd16<System.Runtime.Intrinsics.Vector128`1>(AX) V21 tmp8          $VN.Void
N002 (  5, 12) [000024] n---G------                         │  │  │  │  └──▌  IND       simd16 <l:$1c2, c:$103>
N001 (  3, 10) [000023] H----------                         │  │  │  │     └──▌  CNS_INT(h) long   0x275000091a8 static Fseq[s_v128_int_22] $184
N004 (  3,  3) [000071] ----------- arg0 in rcx             │  │  │  ├──▌  LCL_ADDR  long   V21 tmp8         [+0] $242
N005 (  1,  1) [000080] ----------- arg1 in rdx             │  │  │  └──▌  CNS_INT   int    2 $45
N007 (  1,  1) [000033] -----------                         │  │  └──▌  CNS_INT   int    22 $4b
N009 (  1,  1) [000039] -----------                         │  └──▌  CNS_INT   int    5 $4c
N011 (  1,  1) [000041] -----------                         └──▌  CNS_INT   int    8 $4d

Since [000064] doesn't have the GT_ASG side-effect flag, we hit this assert. The comment says that we should be considering the side-effect of GT_CALLregardless of them_flags` passed, but code doesn't seem to be behaving that way.

runtime/src/coreclr/jit/gentree.cpp

Lines 16780 to 16783 in f18d6b6

	// Generally all GT_CALL nodes are considered to have side-effects.
	// So if we get here it must be a helper call that we decided it does
	// not have side effects that we needed to keep.
	assert(!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER));

[jakobbotsch]

Since [000064] doesn't have the GT_ASG side-effect flag, we hit this assert.

It does have GTF_ASG in your dump (A flag). I guess the caller is optRemoveRangeCheck that is trying to extract just GTF_ASG, which seems odd. I would assume it should extract all side effects, since it will otherwise discard some side effects (like exception throws or calls here).

It looks like in this case we are looking at GetElement(x, y & 5), which creates a bounds check that (y & 5) < 8. We are able to prove that this bounds check always passes, but y involves the call that needs to be extracted.

I'm not sure why we wouldn't see the same issue with regular array indexing, like for example x[M() % x.Length] or x[M() & 5] for a x with a known array length, might be worth to check that out.

[jakobbotsch]

I'm not sure why we wouldn't see the same issue with regular array indexing, like for example x[M() % x.Length] or x[M() & 5] for a x with a known array length, might be worth to check that out.

Looks like fgMorphIndexAddr always spills complex indexing expressions to a new local, so that's why we don't see it for normal indexing. We don't see it for Span indexing because even if the result is discarded, we still end up with a pattern that blocks forward sub.

[BruceForstall]

@jakobbotsch Can you take this one?