From 972b88a90c409f960298e3402c06cb6783a3f592 Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Wed, 5 Nov 2025 15:22:46 +0000 Subject: [PATCH 1/5] arm64: Fix negs compare to minus int.MinValue * Fixes #121294 --- src/coreclr/jit/lower.cpp | 4 +- src/tests/JIT/opt/InstructionCombining/Neg.cs | 52 +++++++++++++++++-- 2 files changed, 49 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 25e5db40c20c6c..155f77243a9881 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4435,10 +4435,10 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) } // Optimize EQ/NE/GT/GE/LT/LE(op_that_sets_zf, 0) into op_that_sets_zf with GTF_SET_FLAGS + SETCC. - // For GT/GE/LT/LE don't allow ADD/SUB, runtime has to check for overflow. + // For GT/GE/LT/LE don't allow ADD/SUB/NEG, runtime has to check for overflow. LIR::Use use; if (((cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag()) || - (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && !op1->OperIs(GT_ADD, GT_SUB) && + (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && !op1->OperIs(GT_ADD, GT_SUB, GT_NEG) && op1->SupportsSettingResultFlags())) && BlockRange().TryGetUse(cmp, &use) && IsProfitableToSetZeroFlag(op1)) { diff --git a/src/tests/JIT/opt/InstructionCombining/Neg.cs b/src/tests/JIT/opt/InstructionCombining/Neg.cs index b6a3e8ab8a9f07..3086d3976739db 100644 --- a/src/tests/JIT/opt/InstructionCombining/Neg.cs +++ b/src/tests/JIT/opt/InstructionCombining/Neg.cs @@ -115,6 +115,16 @@ public static int CheckNeg() fail = true; } + if (NegsGreaterThanIntMinValue()) + { + fail = true; + } + + if (NegsGreaterThanLongMinValue()) + { + fail = true; + } + if (fail) { return 101; @@ -266,33 +276,65 @@ static bool NegsBinOpSingleLine(int a, int b) //ARM64-FULL-LINE: negs {{w[0-9]+}}, {{w[0-9]+}}, LSL #1 return (-(a >> 1) != 0) | (-(b << 1) != 0); } - + [MethodImpl(MethodImplOptions.NoInlining)] static bool NegsGreaterThan(int a) { - //ARM64-FULL-LINE: negs {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: neg {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, #0 return -a > 0; } [MethodImpl(MethodImplOptions.NoInlining)] static bool NegsGreaterThanEq(int a) { - //ARM64-FULL-LINE: negs {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: neg {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, #0 return -a >= 0; } [MethodImpl(MethodImplOptions.NoInlining)] static bool NegsLessThan(int a) { - //ARM64-FULL-LINE: negs {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: neg {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: lsr {{w[0-9]+}}, {{w[0-9]+}}, #31 return -a < 0; } [MethodImpl(MethodImplOptions.NoInlining)] static bool NegsLessThanEq(int a) { - //ARM64-FULL-LINE: negs {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: neg {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, #0 return -a <= 0; } + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool NegsGreaterThanIntMinValue() + { + //ARM64-FULL-LINE: neg {{w[0-9]+}}, {{w[0-9]+}} + //ARM64-FULL-LINE: cmp {{w[0-9]+}}, #0 + return -IntMinValue() > 0; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static bool NegsGreaterThanLongMinValue() + { + //ARM64-FULL-LINE: neg {{x[0-9]+}}, {{x[0-9]+}} + //ARM64-FULL-LINE: cmp {{x[0-9]+}}, #0 + return -LongMinValue() > 0; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static int IntMinValue() + { + return int.MinValue; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + static long LongMinValue() + { + return long.MinValue; + } } } From 0208da9035bde397556993cc10fd1a14f176fc1c Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Fri, 7 Nov 2025 13:33:42 +0000 Subject: [PATCH 2/5] Remove SupportsSettingResultFlags() --- src/coreclr/jit/gentree.cpp | 33 ++------------------------------- src/coreclr/jit/gentree.h | 2 -- src/coreclr/jit/lower.cpp | 4 +--- 3 files changed, 3 insertions(+), 36 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0384b257934c7d..eb370393087749 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -19887,11 +19887,6 @@ bool GenTree::IsArrayAddr(GenTreeArrAddr** pArrAddr) // bool GenTree::SupportsSettingZeroFlag() { - if (SupportsSettingResultFlags()) - { - return true; - } - #if defined(TARGET_XARCH) if (OperIs(GT_LSH, GT_RSH, GT_RSZ, GT_ROL, GT_ROR)) { @@ -19910,42 +19905,18 @@ bool GenTree::SupportsSettingZeroFlag() return true; } #endif -#endif - - return false; -} - -//------------------------------------------------------------------------ -// SupportsSettingResultFlags: Returns true if this is an arithmetic operation -// whose codegen supports setting the carry, overflow, zero and sign flags based -// on the result of the operation. -// -// Return Value: -// True if so. A false return does not imply that codegen for the node will -// not trash the result flags. -// -// Remarks: -// For example, for GT (AND x y) 0, arm64 can emit instructions that -// directly set the flags after the 'AND' and thus no comparison is needed. -// -// The backend expects any node for which the flags will be consumed to be -// marked with GTF_SET_FLAGS. -// -bool GenTree::SupportsSettingResultFlags() -{ -#if defined(TARGET_ARM64) +#elif defined(TARGET_ARM64) if (OperIs(GT_AND, GT_AND_NOT)) { return true; } - // We do not support setting result flags if neg has a contained mul + // We do not support setting zero flag for madd/msub. if (OperIs(GT_NEG) && (!gtGetOp1()->OperIs(GT_MUL) || !gtGetOp1()->isContained())) { return true; } - // We do not support setting result flags for madd/msub. if (OperIs(GT_ADD, GT_SUB) && (!gtGetOp2()->OperIs(GT_MUL) || !gtGetOp2()->isContained())) { return true; diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 100f17301ea267..b76a27879a4a7e 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2070,8 +2070,6 @@ struct GenTree bool SupportsSettingZeroFlag(); - bool SupportsSettingResultFlags(); - // These are only used for dumping. // The GetRegNum() is only valid in LIR, but the dumping methods are not easily // modified to check this. diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 155f77243a9881..d44ed4dffedb52 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4435,11 +4435,9 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) } // Optimize EQ/NE/GT/GE/LT/LE(op_that_sets_zf, 0) into op_that_sets_zf with GTF_SET_FLAGS + SETCC. - // For GT/GE/LT/LE don't allow ADD/SUB/NEG, runtime has to check for overflow. LIR::Use use; if (((cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag()) || - (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && !op1->OperIs(GT_ADD, GT_SUB, GT_NEG) && - op1->SupportsSettingResultFlags())) && + (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && op1->OperIs(GT_AND, GT_AND_NOT))) && BlockRange().TryGetUse(cmp, &use) && IsProfitableToSetZeroFlag(op1)) { op1->gtFlags |= GTF_SET_FLAGS; From 76616296cac3c92cf34ba38dbb9fb7b4d60b561e Mon Sep 17 00:00:00 2001 From: Jonathan Davies Date: Tue, 18 Nov 2025 10:44:35 +0000 Subject: [PATCH 3/5] Add SupportsSettingFlagsAsCompareToZero --- src/coreclr/jit/gentree.cpp | 13 +++++++++++++ src/coreclr/jit/gentree.h | 2 ++ src/coreclr/jit/lower.cpp | 2 +- 3 files changed, 16 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 0fe87f38f0b670..dacac8b3f50d60 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -19938,6 +19938,19 @@ bool GenTree::SupportsSettingZeroFlag() return false; } +//------------------------------------------------------------------------ +// SupportsSettingFlagsAsCompareToZero: Returns true if we support setting +// flags for compare to zero operations. +// +bool GenTree::SupportsSettingFlagsAsCompareToZero() +{ +#if defined(TARGET_ARM64) + return true; +#else + return false; +#endif +} + //------------------------------------------------------------------------ // Create: Create or retrieve a field sequence for the given field handle. // diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index df770aef446503..e581e8a1147e25 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -2070,6 +2070,8 @@ struct GenTree bool SupportsSettingZeroFlag(); + bool SupportsSettingFlagsAsCompareToZero(); + // These are only used for dumping. // The GetRegNum() is only valid in LIR, but the dumping methods are not easily // modified to check this. diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d44ed4dffedb52..888f3ee6fe06a7 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4437,7 +4437,7 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) // Optimize EQ/NE/GT/GE/LT/LE(op_that_sets_zf, 0) into op_that_sets_zf with GTF_SET_FLAGS + SETCC. LIR::Use use; if (((cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag()) || - (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && op1->OperIs(GT_AND, GT_AND_NOT))) && + (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && op1->OperIs(GT_AND, GT_AND_NOT) && op1->SupportsSettingFlagsAsCompareToZero())) && BlockRange().TryGetUse(cmp, &use) && IsProfitableToSetZeroFlag(op1)) { op1->gtFlags |= GTF_SET_FLAGS; From a664e3ccdb12f7947852102aa9d2fa48fcc78ca3 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 25 Nov 2025 13:20:04 +0100 Subject: [PATCH 4/5] Move checks into SupportsSettingFlagsAsCompareToZero --- src/coreclr/jit/gentree.cpp | 4 ++-- src/coreclr/jit/lower.cpp | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index dacac8b3f50d60..2dbcf1564445fe 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -19944,8 +19944,8 @@ bool GenTree::SupportsSettingZeroFlag() // bool GenTree::SupportsSettingFlagsAsCompareToZero() { -#if defined(TARGET_ARM64) - return true; +#if defined(TARGET_ARMARCH) + return OperIs(GT_AND, GT_AND_NOT); #else return false; #endif diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 888f3ee6fe06a7..d05472e58f989a 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4437,7 +4437,7 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) // Optimize EQ/NE/GT/GE/LT/LE(op_that_sets_zf, 0) into op_that_sets_zf with GTF_SET_FLAGS + SETCC. LIR::Use use; if (((cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag()) || - (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && op1->OperIs(GT_AND, GT_AND_NOT) && op1->SupportsSettingFlagsAsCompareToZero())) && + (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && op1->SupportsSettingFlagsAsCompareToZero())) && BlockRange().TryGetUse(cmp, &use) && IsProfitableToSetZeroFlag(op1)) { op1->gtFlags |= GTF_SET_FLAGS; From 566c9dec2e6d61989437e224066cde993866605b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Tue, 25 Nov 2025 13:20:26 +0100 Subject: [PATCH 5/5] Run jit-format --- src/coreclr/jit/lower.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index d05472e58f989a..53d8938badd34d 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -4437,7 +4437,8 @@ GenTree* Lowering::OptimizeConstCompare(GenTree* cmp) // Optimize EQ/NE/GT/GE/LT/LE(op_that_sets_zf, 0) into op_that_sets_zf with GTF_SET_FLAGS + SETCC. LIR::Use use; if (((cmp->OperIs(GT_EQ, GT_NE) && op2->IsIntegralConst(0) && op1->SupportsSettingZeroFlag()) || - (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && op1->SupportsSettingFlagsAsCompareToZero())) && + (cmp->OperIs(GT_GT, GT_GE, GT_LT, GT_LE) && op2->IsIntegralConst(0) && + op1->SupportsSettingFlagsAsCompareToZero())) && BlockRange().TryGetUse(cmp, &use) && IsProfitableToSetZeroFlag(op1)) { op1->gtFlags |= GTF_SET_FLAGS;