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: VN does not encode base type for CreateScalar #105721

Closed
jakobbotsch opened this issue Jul 30, 2024 · 9 comments · Fixed by #105869
Closed

JIT: VN does not encode base type for CreateScalar #105721

jakobbotsch opened this issue Jul 30, 2024 · 9 comments · Fixed by #105869
Assignees
Labels
area-CodeGen-coreclr CLR JIT compiler in src/coreclr/src/jit and related components such as SuperPMI in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release
Milestone

Comments

@jakobbotsch
Copy link
Member

jakobbotsch commented Jul 30, 2024

// Generated by Fuzzlyn v2.2 on 2024-07-30 18:05:36
// Run on Arm64 Windows
// Seed: 12131813856619591872-vectort,vector64,vector128,armsve
// Reduced from 251.5 KiB to 0.9 KiB in 00:03:25
// Debug: Outputs <-1, 0, 0, 0>
// Release: Outputs <65535, 0, 0, 0>
using System;
using System.Runtime.CompilerServices;
using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;

public class C0
{
    public ulong F6;
}

public class C1
{
    public C0 F1;
    public C1(C0 f1)
    {
        F1 = f1;
    }
}

public class Program
{
    public static short s_21;
    public static C1 s_34 = new C1(new C0());
    public static void Main()
    {
        C0 vr7 = s_34.F1;
        vr7.F6 = (ulong)s_21--;
        var vr8 = (short)1;
        var vr9 = Vector128.CreateScalar(vr8).AsVector();
        var vr10 = Vector128.CreateScalar(s_21).AsVector();
        if (!!Sve.TestFirstTrue(vr9, vr10))
        {
            var vr11 = (int)s_21;
            Vector<int> vr12 = Vector128.CreateScalar(vr11).AsVector();
            System.Console.WriteLine(vr12);
        }
    }
}

cc @dotnet/jit-contrib @dotnet/arm64-contrib

(This one may potentially not be SVE related -- not sure.)

@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 Jul 30, 2024
@dotnet-policy-service dotnet-policy-service bot added the untriaged New issue has not been triaged by the area owner label Jul 30, 2024
@jakobbotsch jakobbotsch added arm-sve Work related to arm64 SVE/SVE2 support and removed untriaged New issue has not been triaged by the area owner labels Jul 30, 2024
Copy link
Contributor

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

@jakobbotsch jakobbotsch added this to the 9.0.0 milestone Jul 30, 2024
@a74nh
Copy link
Contributor

a74nh commented Jul 31, 2024

priority:2 for RC1 snap : Functionality issue

@a74nh a74nh added the Priority:2 Work that is important, but not critical for the release label Jul 31, 2024
@a74nh
Copy link
Contributor

a74nh commented Aug 1, 2024

Looks like this is a CSE issue.

        var vr8 = (short)1;
        var vr9 = Vector128.CreateScalar(vr8).AsVector();
        var vr10 = Vector128.CreateScalar(s_21).AsVector();
	if (Sve.TestFirstTrue(vr9, vr10))

Due to vr8 being a short, vr10 is a vector<short>.

This can be seen whenvr10 is created like this:

            ldrsh   w0, [x1]
            ins     v16.h[0], w0
            str     q16, [fp, #0x10]	// [V05 cse0]

Later, inside the if block:

            var vr11 = (int)s_21;
            Vector<int> vr12 = Vector128.CreateScalar(vr11).AsVector();

vr12 simply reuses v05:

            ldr     q16, [fp, #0x10]	// [V05 cse0]
            str     q16, [x0, #0x08]

Which is incorrect.

@a74nh
Copy link
Contributor

a74nh commented Aug 1, 2024

var vr10 = Vector128.CreateScalar(s_21).AsVector();

***** BB01 [0000]
STMT00005 ( 0x01E[E-] ... 0x038 )
               [000039] DACXG------                         *  STORE_LCL_VAR simd16<System.Numerics.Vector`1> V00 loc0         
               [000038] --CXG------                         \--*  HWINTRINSIC simd16 short CreateScalar
               [000037] --CXG------                            \--*  COMMA     short 
               [000036] H-CXG------                               +--*  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE
               [000035] H---------- arg0                          |  \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program
               [000033] I---G------                               \--*  IND       short 
               [000032] H----------                                  \--*  CNS_INT(h) long   0xf4e94802b148 static Fseq[s_21]

Vector<int> vr12 = Vector128.CreateScalar(vr11).AsVector();

***** BB02 [0001]
STMT00009 ( ??? ... ??? )
               [000063] -ACXG------                         *  STOREIND  simd16 (copy)
               [000062] -----------                         +--*  ADD       byref 
               [000060] -----------                         |  +--*  LCL_VAR   ref    V04 tmp3         
               [000061] -----------                         |  \--*  CNS_INT   long   8
               [000056] --CXG------                         \--*  HWINTRINSIC simd16 int CreateScalar
               [000055] --CXG------                            \--*  COMMA     short 
               [000054] H-CXG------                               +--*  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE
               [000053] H---------- arg0                          |  \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program
               [000051] I---G------                               \--*  IND       short 
               [000050] H----------                                  \--*  CNS_INT(h) long   0xf4e94802b148 static Fseq[s_21]

@jakobbotsch
Copy link
Member Author

Small repro that repros on win-x64 as well:

using System;
using System.Runtime.Intrinsics;

public class Program
{
    public static void Main()
    {
        new Program().Foo();
    }

    public short s_21 = -1;
    private void Foo()
    {
        Vector128<short> v1 = Vector128.CreateScalar<short>(s_21);
        Vector128<int> v2 = Vector128.CreateScalar<int>(s_21);
        Console.WriteLine(v1);
        Console.WriteLine(v2);
    }
}

Looks like we should be encoding the base type in CreateScalar. I can grab this one, thanks for investigating @a74nh!

@jakobbotsch jakobbotsch removed the arm-sve Work related to arm64 SVE/SVE2 support label Aug 1, 2024
@jakobbotsch jakobbotsch changed the title JIT SVE: Invalid result with Sve.TestFirstTrue JIT: VN does not encode base type for CreateScalar Aug 1, 2024
@a74nh
Copy link
Contributor

a74nh commented Aug 1, 2024

************ Trees at start of optValnumCSE_Heuristic()

------------ BB01 [0000] [000..041) -> BB03(0.5),BB02(0.5) (cond), preds={} succs={BB02,BB03}

***** BB01 [0000]
STMT00001 ( 0x000[E-] ... 0x013 )
N009 ( 26, 31)              [000015] DACXG------                         *  STORE_LCL_VAR ref    V03 tmp2         d:2 <l:$18d, c:$18e>
N008 ( 26, 31)              [000007] --CXG------                         \--*  IND       ref    <l:$18a, c:$18c>
N007 ( 25, 32)              [000068] --CXG--N---                            \--*  ADD       byref  <l:$203, c:$204>
N005 ( 23, 29)              [000005] --CXG------                               +--*  COMMA     ref    <l:$182, c:$183>
N002 ( 17, 15) CSE #02 (def)[000004] H-CXG------                               |  +--*  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE $200
N001 (  3, 12) CSE #01 (def)[000003] H---------- arg0 in x0                    |  |  \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program $100
N004 (  6, 14)              [000001] I---G------                               |  \--*  IND       ref    <l:$181, c:$141>
N003 (  3, 12)              [000000] H----------                               |     \--*  CNS_INT(h) long   0xf4a9380001c0 static Fseq[s_34] $101
N006 (  1,  2)              [000067] -----------                               \--*  CNS_INT   long   8 Fseq[F1] $240

***** BB01 [0000]
STMT00000 ( 0x000[E-] ... ??? )
N006 ( 24, 30)              [000014] DACXG------                         *  STORE_LCL_VAR int    V02 tmp1         d:2 $18f
N005 ( 24, 30)              [000013] --CXG------                         \--*  COMMA     short  <l:$2c1, c:$2c2>
N002 ( 17, 15) CSE #02 (use)[000012] H-CXG------                            +--*  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE $200
N001 (  3, 12) CSE #01 (use)[000011] H---------- arg0 in x0                 |  \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program $100
N004 (  7, 15)              [000009] I---G------                            \--*  IND       short  <l:$2c0, c:$300>
N003 (  3, 12) CSE #03 (def)[000008] H----------                               \--*  CNS_INT(h) long   0xf4e94802b148 static Fseq[s_21] $104

***** BB01 [0000]
STMT00002 ( ??? ... ??? )
N002 ( 17, 15) CSE #02 (use)[000024] H-CXG------                         *  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE $200
N001 (  3, 12) CSE #01 (use)[000023] H---------- arg0 in x0              \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program $100

***** BB01 [0000]
STMT00003 ( ??? ... ??? )
N005 ( 11, 20)              [000026] IA--G---R--                         *  STOREIND  short  $VN.Void
N004 (  3, 12) CSE #03 (use)[000025] H----------                         +--*  CNS_INT(h) long   0xf4e94802b148 static Fseq[s_21] $104
N003 (  3,  4)              [000020] -----------                         \--*  ADD       int    <l:$342, c:$343>
N001 (  1,  1)              [000018] -----------                            +--*  LCL_VAR   int    V02 tmp1         u:2 <l:$340, c:$341>
N002 (  1,  2)              [000019] -----------                            \--*  CNS_INT   int    -1 $41

***** BB01 [0000]
STMT00004 ( ??? ... 0x019 )
N006 (  7,  7)              [000029] -A-XG---R--                         *  STOREIND  long   <l:$192, c:$193>
N005 (  3,  4)              [000072] -------N---                         +--*  ADD       byref  <l:$205, c:$206>
N003 (  1,  1)              [000016] -----------                         |  +--*  LCL_VAR   ref    V03 tmp2         u:2 (last use) <l:$184, c:$142>
N004 (  1,  2)              [000071] -----------                         |  \--*  CNS_INT   long   8 Fseq[F6] $240
N002 (  2,  3)              [000027] -----------                         \--*  CAST      long <- int <l:$3c0, c:$3c1>
N001 (  1,  1)              [000017] -----------                            \--*  LCL_VAR   int    V02 tmp1         u:2 (last use) <l:$340, c:$341>

***** BB01 [0000]
STMT00006 ( ??? ... 0x03F )
N015 ( 37, 43)              [000048] --CXG------                         *  JTRUE     void   $18f
N014 ( 35, 41)              [000047] J-CXG--N---                         \--*  EQ        int    <l:$34a, c:$34b>
N012 ( 33, 38)              [000041] --CXG---R--                            +--*  HWINTRINSIC int    short TestFirstTrue <l:$346, c:$347>
N011 (  5,  4)              [000045] -----------                            |  +--*  HWINTRINSIC mask   short ConvertVectorToMask $502
N009 (  1,  1)              [000044] -----------                            |  |  +--*  HWINTRINSIC mask   short CreateTrueMaskAll $4c0
N010 (  3,  2)              [000031] -----------                            |  |  \--*  CNS_VEC   simd16<0x00000001, 0x00000000, 0x00000000, 0x00000000> $580
N008 ( 27, 33)              [000043] --CXG---R--                            |  \--*  HWINTRINSIC mask   short ConvertVectorToMask <l:$540, c:$541>
N007 (  1,  1)              [000042] -----------                            |     +--*  HWINTRINSIC mask   short CreateTrueMaskAll $4c0
N006 ( 25, 31) CSE #06 (def)[000038] --CXG------                            |     \--*  HWINTRINSIC simd16 short CreateScalar <l:$480, c:$481>
N005 ( 24, 30) CSE #05 (def)[000037] --CXG------                            |        \--*  COMMA     short  <l:$2c4, c:$2c5>
N002 ( 17, 15) CSE #02 (use)[000036] H-CXG------                            |           +--*  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE $200
N001 (  3, 12) CSE #01 (use)[000035] H---------- arg0 in x0                 |           |  \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program $100
N004 (  7, 15) CSE #04 (def)[000033] I---G------                            |           \--*  IND       short  <l:$2c3, c:$301>
N003 (  3, 12) CSE #03 (use)[000032] H----------                            |              \--*  CNS_INT(h) long   0xf4e94802b148 static Fseq[s_21] $104
N013 (  1,  2)              [000046] -----------                            \--*  CNS_INT   int    0 $43

------------ BB02 [0001] [041..05A) (return), preds={BB01} succs={}

***** BB02 [0001]
STMT00008 ( 0x041[E-] ... 0x055 )
N003 ( 17, 15)              [000059] DAC--------                         *  STORE_LCL_VAR ref    V04 tmp3         d:2 $VN.Void
N002 ( 17, 15)              [000058] --C--------                         \--*  CALL help ref    CORINFO_HELP_NEWSFAST $195
N001 (  3, 12)              [000057] H---------- arg0 in x0                 \--*  CNS_INT(h) long   0xf4e949f26b78 class System.Numerics.Vector`1[int] $107

***** BB02 [0001]
STMT00009 ( ??? ... ??? )
N010 ( 30, 35)              [000063] -ACXG---R--                         *  STOREIND  simd16 (copy) $18f
N009 (  3,  4)              [000062] -------N---                         +--*  ADD       byref  $207
N007 (  1,  1)              [000060] -----------                         |  +--*  LCL_VAR   ref    V04 tmp3         u:2 $195
N008 (  1,  2)              [000061] -----------                         |  \--*  CNS_INT   long   8 $240
N006 ( 25, 31) CSE #06 (use)[000056] --CXG------                         \--*  HWINTRINSIC simd16 int CreateScalar <l:$480, c:$482>
N005 ( 24, 30) CSE #05 (use)[000055] --CXG------                            \--*  COMMA     short  <l:$2c4, c:$2c6>
N002 ( 17, 15) CSE #02 (use)[000054] H-CXG------                               +--*  CALL help byref  CORINFO_HELP_GET_NONGCSTATIC_BASE $200
N001 (  3, 12) CSE #01 (use)[000053] H---------- arg0 in x0                    |  \--*  CNS_INT(h) long   0xf4e949e72ad8 class Program $100
N004 (  7, 15) CSE #04 (use)[000051] I---G------                               \--*  IND       short  <l:$2c3, c:$302>
N003 (  3, 12) CSE #03 (use)[000050] H----------                                  \--*  CNS_INT(h) long   0xf4e94802b148 static Fseq[s_21] $104

***** BB02 [0001]
STMT00010 ( ??? ... ??? )
N003 ( 21,  8)              [000066] --CXG------                         *  CALL      void   System.Console:WriteLine(System.Object) $VN.Void
N002 (  7,  5)              [000065] ----------- arg0 in x0              \--*  BOX       ref    $195
N001 (  1,  1)              [000064] -----------                            \--*  LCL_VAR   ref    V04 tmp3         u:2 (last use) $195

------------ BB03 [0002] [05A..05B) (return), preds={BB01} succs={}

***** BB03 [0002]
STMT00007 ( 0x05A[E-] ... 0x05A )
N001 (  0,  0)              [000049] -----------                         *  RETURN    void   $VN.Void

The def and use of CSE #06 don't match.
If CSE #06 is removed then I think CSE #05 would get applied instead and everything would work.

@a74nh
Copy link
Contributor

a74nh commented Aug 1, 2024

Looking at the CSE code I'm a little lost.
Is this a failure when creating the hash table (ie - the return type of the node is not taken into account correctly)? Or does there need to be additional checks when an expression matches an existing item in the hash table?

@jakobbotsch
Copy link
Member Author

The problem here is in value numbering -- VN should not be giving [000056] and [000038] the same VNs (they both get l:$480). That indicates they have the same value, and CSE works off of that.

For some intrinsics their operation is altered by the type they operate on, in this case short/int. VN has to encode that sometimes, and it forgets to do that correctly in this scenario. I'm working on a fix.

@a74nh
Copy link
Contributor

a74nh commented Aug 1, 2024

Looks like we should be encoding the base type in CreateScalar. I can grab this one, thanks for investigating @a74nh!

Didn't spot this earlier! Thanks for taking it.

@jakobbotsch jakobbotsch assigned jakobbotsch and unassigned a74nh Aug 1, 2024
@dotnet-policy-service dotnet-policy-service bot added the in-pr There is an active PR which will close this issue when it is merged label Aug 2, 2024
@github-actions github-actions bot locked and limited conversation to collaborators Sep 2, 2024
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 in-pr There is an active PR which will close this issue when it is merged Priority:2 Work that is important, but not critical for the release
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants