From 12755c105351f706f8d8f58c647a4ec566ceb512 Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Tue, 11 Mar 2025 10:23:31 +0000 Subject: [PATCH 1/7] Fix assertions related to combining `cmn (extended-register)` (#113337) Fixes issues highlighted by Fuzzlyn, related to erroneously combining cast=>negate=>compare into `cmn (extended-register)` when the second operand is an integral constant. --- src/coreclr/jit/lowerarmarch.cpp | 4 +- .../JitBlue/Runtime_113337/Runtime_113337.cs | 39 +++++++++++++++++++ .../Runtime_113337/Runtime_113337.csproj | 8 ++++ 3 files changed, 49 insertions(+), 2 deletions(-) create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs create mode 100644 src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.csproj diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 89b01b20b36fcb..8b41fc6987600b 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -404,9 +404,9 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN return false; } - if (childNode->gtGetOp1()->OperIs(GT_CAST)) + if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->IsIntegralConst()) { - // Grab the cast as well, we can contain this with cmn. + // Grab the cast as well, we can contain this with cmn (extended-register). GenTreeCast* cast = childNode->gtGetOp1()->AsCast(); assert(!cast->gtOverflow()); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs new file mode 100644 index 00000000000000..35b863d0dd28b7 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs @@ -0,0 +1,39 @@ +using System; +using System.Numerics; +using System.Runtime.Intrinsics; +using System.Runtime.Intrinsics.Arm; +using System.Runtime.CompilerServices; +using Xunit; + +public class Program +{ + static sbyte[] s_7; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void issue1() + { + var vr9 = Vector64.Create(0); + if ((2147483647 == (-(int)AdvSimd.Extract(vr9, 1)))) + { + s_7 = s_7; + } + } + + static int[][] s_2; + static bool s_3; + + [MethodImpl(MethodImplOptions.NoInlining)] + static void issue2() + { + s_3 ^= (2021486855 != (-(long)s_2[0][0])); + } + + [Fact] + public static int TestEntryPoint() + { + // Checking for successful compilation + issue1(); + issue2(); + return 100; + } +} \ No newline at end of file diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.csproj new file mode 100644 index 00000000000000..15edd99711a1a4 --- /dev/null +++ b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.csproj @@ -0,0 +1,8 @@ + + + True + + + + + \ No newline at end of file From 4e6d417181f1fc094d0157619af97a716357323c Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Tue, 11 Mar 2025 17:01:30 +0000 Subject: [PATCH 2/7] Fix test and address code review comments --- src/coreclr/jit/lowerarmarch.cpp | 5 ++++- .../JitBlue/Runtime_113337/Runtime_113337.cs | 15 +++++++++++---- 2 files changed, 15 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 8b41fc6987600b..ca23d4ef2abc12 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -404,7 +404,10 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN return false; } - if (childNode->gtGetOp1()->OperIs(GT_CAST) && !parentNode->gtGetOp2()->IsIntegralConst()) + GenTree* otherNode = + (childNode == parentNode->gtGetOp2()) ? parentNode->gtGetOp1() : parentNode->gtGetOp2(); + + if (childNode->gtGetOp1()->OperIs(GT_CAST) && !otherNode->IsIntegralConst()) { // Grab the cast as well, we can contain this with cmn (extended-register). GenTreeCast* cast = childNode->gtGetOp1()->AsCast(); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs index 35b863d0dd28b7..17e49e8835a875 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs @@ -1,3 +1,5 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. using System; using System.Numerics; using System.Runtime.Intrinsics; @@ -5,7 +7,7 @@ using System.Runtime.CompilerServices; using Xunit; -public class Program +public class Runtime_113337 { static sbyte[] s_7; @@ -25,15 +27,20 @@ static void issue1() [MethodImpl(MethodImplOptions.NoInlining)] static void issue2() { - s_3 ^= (2021486855 != (-(long)s_2[0][0])); + try + { + s_3 ^= (2021486855 != (-(long)s_2[0][0])); + } + catch (NullReferenceException e) + { + } } [Fact] - public static int TestEntryPoint() + public static void TestEntryPoint() { // Checking for successful compilation issue1(); issue2(); - return 100; } } \ No newline at end of file From ee62a3aa643ae4ed047d3e8fb0028bfefe6f8c09 Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Wed, 12 Mar 2025 10:26:40 +0000 Subject: [PATCH 3/7] Fix tests for other architectures --- .../JitBlue/Runtime_113337/Runtime_113337.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs index 17e49e8835a875..b136d11b8ee51e 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs @@ -1,5 +1,6 @@ // Licensed to the .NET Foundation under one or more agreements. // The .NET Foundation licenses this file to you under the MIT license. + using System; using System.Numerics; using System.Runtime.Intrinsics; @@ -14,10 +15,16 @@ public class Runtime_113337 [MethodImpl(MethodImplOptions.NoInlining)] static void issue1() { - var vr9 = Vector64.Create(0); - if ((2147483647 == (-(int)AdvSimd.Extract(vr9, 1)))) + try + { + var vr9 = Vector64.Create(0); + if ((2147483647 == (-(int)AdvSimd.Extract(vr9, 1)))) + { + s_7 = s_7; + } + } + catch (PlatformNotSupportedException e) { - s_7 = s_7; } } From 1f900f00a8c98e75d8f374b87bbf0bdb65ffbba1 Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Wed, 12 Mar 2025 10:27:55 +0000 Subject: [PATCH 4/7] Restrict containment to integer->integer casts, and prevent containing casts that already contain some memory operation --- src/coreclr/jit/lowerarmarch.cpp | 18 ++++++++++++++---- 1 file changed, 14 insertions(+), 4 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index ca23d4ef2abc12..5ea02dac642b97 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -404,13 +404,23 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN return false; } - GenTree* otherNode = - (childNode == parentNode->gtGetOp2()) ? parentNode->gtGetOp1() : parentNode->gtGetOp2(); - - if (childNode->gtGetOp1()->OperIs(GT_CAST) && !otherNode->IsIntegralConst()) + if (childNode->gtGetOp1()->OperIs(GT_CAST)) { // Grab the cast as well, we can contain this with cmn (extended-register). GenTreeCast* cast = childNode->gtGetOp1()->AsCast(); + GenTree* castOp = cast->CastOp(); + + // Cannot contain the cast from floating point. + if (!varTypeIsIntegral(castOp)) + { + return false; + } + + // Cannot contain the cast if it may contain or is already containing a memory operation. + if (IsContainableMemoryOp(castOp) || castOp->isContained()) + { + return false; + } assert(!cast->gtOverflow()); assert(varTypeIsIntegral(cast) && varTypeIsIntegral(cast->CastToType())); From 6079bb405627055068f463466acecd9f87e4fc86 Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Wed, 12 Mar 2025 10:29:07 +0000 Subject: [PATCH 5/7] Formatting --- src/coreclr/jit/lowerarmarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 5ea02dac642b97..0cd5d68eacb6c0 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -407,7 +407,7 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN if (childNode->gtGetOp1()->OperIs(GT_CAST)) { // Grab the cast as well, we can contain this with cmn (extended-register). - GenTreeCast* cast = childNode->gtGetOp1()->AsCast(); + GenTreeCast* cast = childNode->gtGetOp1()->AsCast(); GenTree* castOp = cast->CastOp(); // Cannot contain the cast from floating point. From 7bcb67c575d5d5239d51f827d1a667dbeaa358c8 Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Mon, 24 Mar 2025 16:08:16 +0000 Subject: [PATCH 6/7] Remove check for IsContainableMemoryOp --- src/coreclr/jit/lowerarmarch.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 627a1ecace634d..fdc7dcfcd8d1a3 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -416,8 +416,8 @@ bool Lowering::IsContainableUnaryOrBinaryOp(GenTree* parentNode, GenTree* childN return false; } - // Cannot contain the cast if it may contain or is already containing a memory operation. - if (IsContainableMemoryOp(castOp) || castOp->isContained()) + // Cannot contain the cast if it already contains it's CastOp. + if (castOp->isContained()) { return false; } From 83ebe281928009c0c2a3aa4737d620efaec4eed9 Mon Sep 17 00:00:00 2001 From: Sebastian Nickolls Date: Wed, 26 Mar 2025 13:12:31 +0000 Subject: [PATCH 7/7] Fix interaction with CCMP optimization --- src/coreclr/jit/lowerarmarch.cpp | 4 +++ .../JitBlue/Runtime_113337/Runtime_113337.cs | 25 +++++++++++++++++++ 2 files changed, 29 insertions(+) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index fdc7dcfcd8d1a3..fdfef546fde5a4 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -3200,11 +3200,15 @@ bool Lowering::TryLowerAndOrToCCMP(GenTreeOp* tree, GenTree** next) // GenCondition cond1; if (op2->OperIsCmpCompare() && varTypeIsIntegralOrI(op2->gtGetOp1()) && IsInvariantInRange(op2, tree) && + (op2->gtGetOp1()->IsIntegralConst() || !op2->gtGetOp1()->isContained()) && + (op2->gtGetOp2() == nullptr || op2->gtGetOp2()->IsIntegralConst() || !op2->gtGetOp2()->isContained()) && TryLowerConditionToFlagsNode(tree, op1, &cond1)) { // Fall through, converting op2 to the CCMP } else if (op1->OperIsCmpCompare() && varTypeIsIntegralOrI(op1->gtGetOp1()) && IsInvariantInRange(op1, tree) && + (op1->gtGetOp1()->IsIntegralConst() || !op1->gtGetOp1()->isContained()) && + (op1->gtGetOp2() == nullptr || op1->gtGetOp2()->IsIntegralConst() || !op1->gtGetOp2()->isContained()) && TryLowerConditionToFlagsNode(tree, op2, &cond1)) { std::swap(op1, op2); diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs index b136d11b8ee51e..bb0d2c8aa4529b 100644 --- a/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs +++ b/src/tests/JIT/Regression/JitBlue/Runtime_113337/Runtime_113337.cs @@ -8,6 +8,8 @@ using System.Runtime.CompilerServices; using Xunit; +#pragma warning disable SYSLIB5003 // Allow experimental SVE + public class Runtime_113337 { static sbyte[] s_7; @@ -43,11 +45,34 @@ static void issue2() } } + static Vector[] s_4; + [MethodImpl(MethodImplOptions.NoInlining)] + static void issue3() + { + try + { + var vr3 = Vector.Create(1); + var vr4 = (short)0; + var vr5 = Vector128.CreateScalar(vr4).AsVector(); + if ((Sve.TestFirstTrue(vr3, vr5) | (3268100580U != (-(uint)Sve.SaturatingDecrementBy16BitElementCount(0, 1))))) + { + s_4[0] = s_4[0]; + } + } + catch (PlatformNotSupportedException e) + { + } + catch (NullReferenceException e) + { + } + } + [Fact] public static void TestEntryPoint() { // Checking for successful compilation issue1(); issue2(); + issue3(); } } \ No newline at end of file