From b642b81c08b96c442d8ab889d787fd33c9a1bf43 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 26 Nov 2020 02:38:01 +0300 Subject: [PATCH 1/5] Don't optimize singed comparisons --- src/coreclr/jit/codegenxarch.cpp | 3 ++- .../opt/JitMinOpts/Regression/GitHub_35627.il | 17 +++++++++++++++++ .../JitMinOpts/Regression/GitHub_35627.ilproj | 10 ++++++++++ 3 files changed, 29 insertions(+), 1 deletion(-) create mode 100644 src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il create mode 100644 src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 6f1ba9ab9bb8b6..78a31a3fe03327 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -5960,7 +5960,8 @@ void CodeGen::genCompareInt(GenTree* treeNode) // Optimize "x<0" and "x>=0" to "x>>31" if "x" is not a jump condition and in a reg. // Morph/Lowering are responsible to rotate "00" so we won't handle it here. - if ((targetSize >= 4) && (op1Size >= 4) && (targetReg != REG_NA) && tree->OperIs(GT_LT, GT_GE)) + if ((targetSize >= 4) && (op1Size >= 4) && (targetReg != REG_NA) && tree->OperIs(GT_LT, GT_GE) && + !tree->IsUnsigned()) { if (targetReg != op1->GetRegNum()) { diff --git a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il new file mode 100644 index 00000000000000..c6bc37200f2be5 --- /dev/null +++ b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il @@ -0,0 +1,17 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. + +.assembly extern mscorlib { } +.assembly GitHub_35627 {} +.module GitHub_35627.exe +.method public static int32 Main() cil managed +{ + .entrypoint + .maxstack 5 + ldc.i4.s 100 + ldc.i4.0 + ldc.i4.m1 + cgt.un + add + ret +} diff --git a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj new file mode 100644 index 00000000000000..9d516c2101f63c --- /dev/null +++ b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj @@ -0,0 +1,10 @@ + + + Exe + Full + False + + + + + From 9f9ee42d89ac427f8efef8a04918612aaa675050 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 26 Nov 2020 02:59:16 +0300 Subject: [PATCH 2/5] use issue's ID --- .../Regression/{GitHub_35627.il => GitHub_42719.il} | 5 +++-- .../Regression/{GitHub_35627.ilproj => GitHub_42719.ilproj} | 2 +- 2 files changed, 4 insertions(+), 3 deletions(-) rename src/tests/JIT/opt/JitMinOpts/Regression/{GitHub_35627.il => GitHub_42719.il} (75%) rename src/tests/JIT/opt/JitMinOpts/Regression/{GitHub_35627.ilproj => GitHub_42719.ilproj} (83%) diff --git a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.il similarity index 75% rename from src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il rename to src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.il index c6bc37200f2be5..07f37c96b9b193 100644 --- a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.il +++ b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.il @@ -2,12 +2,13 @@ // The .NET Foundation licenses this file to you under the MIT license. .assembly extern mscorlib { } -.assembly GitHub_35627 {} -.module GitHub_35627.exe +.assembly GitHub_42719 {} +.module GitHub_42719.exe .method public static int32 Main() cil managed { .entrypoint .maxstack 5 + // "0 u> -1" shouldn't be optimized into "-1 >> 31" ldc.i4.s 100 ldc.i4.0 ldc.i4.m1 diff --git a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj similarity index 83% rename from src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj rename to src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj index 9d516c2101f63c..6aaf91d1f0f3be 100644 --- a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_35627.ilproj +++ b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj @@ -5,6 +5,6 @@ False - + From f52953f7af9671d78b47e88ffe17eeb37b2799d9 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Thu, 26 Nov 2020 03:46:42 +0300 Subject: [PATCH 3/5] Address feedback --- src/coreclr/jit/codegenxarch.cpp | 40 +++++++++---------- .../JitMinOpts/Regression/GitHub_42719.ilproj | 1 - .../Regression/GitHub_42719_opt.ilproj | 9 +++++ 3 files changed, 28 insertions(+), 22 deletions(-) create mode 100644 src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719_opt.ilproj diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 78a31a3fe03327..17ef27b2633f5d 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -5955,30 +5955,28 @@ void CodeGen::genCompareInt(GenTree* treeNode) } else if (op1->isUsedFromReg() && op2->IsIntegralConst(0)) { - emitAttr targetSize = emitActualTypeSize(op1->TypeGet()); - emitAttr op1Size = emitActualTypeSize(op1->TypeGet()); - - // Optimize "x<0" and "x>=0" to "x>>31" if "x" is not a jump condition and in a reg. - // Morph/Lowering are responsible to rotate "00" so we won't handle it here. - if ((targetSize >= 4) && (op1Size >= 4) && (targetReg != REG_NA) && tree->OperIs(GT_LT, GT_GE) && - !tree->IsUnsigned()) + if (compiler->opts.OptimizationEnabled()) { - if (targetReg != op1->GetRegNum()) - { - inst_RV_RV(INS_mov, targetReg, op1->GetRegNum(), op1->TypeGet()); - } - if (tree->OperIs(GT_GE)) + emitAttr op1Size = emitActualTypeSize(op1->TypeGet()); + assert(static_cast(op1Size) >= 32); + + // Optimize "x<0" and "x>=0" to "x>>31" if "x" is not a jump condition and in a reg. + // Morph/Lowering are responsible to rotate "00" so we won't handle it here. + if ((targetReg != REG_NA) && tree->OperIs(GT_LT, GT_GE) && !tree->IsUnsigned()) { - // emit "not" for "x>=0" case - inst_RV(INS_not, targetReg, tree->TypeGet(), op1Size); + if (targetReg != op1->GetRegNum()) + { + inst_RV_RV(INS_mov, targetReg, op1->GetRegNum(), op1->TypeGet()); + } + if (tree->OperIs(GT_GE)) + { + // emit "not" for "x>=0" case + inst_RV(INS_not, targetReg, op1->TypeGet()); + } + inst_RV_IV(INS_shr_N, targetReg, (int)op1Size * 8 - 1, op1Size); + genProduceReg(tree); + return; } - inst_RV_IV(INS_shr_N, targetReg, (int)op1Size * 8 - 1, op1Size); - genProduceReg(tree); - return; - } - - if (compiler->opts.OptimizationEnabled()) - { canReuseFlags = true; } diff --git a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj index 6aaf91d1f0f3be..ad6d87becfd14d 100644 --- a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj +++ b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719.ilproj @@ -1,7 +1,6 @@ Exe - Full False diff --git a/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719_opt.ilproj b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719_opt.ilproj new file mode 100644 index 00000000000000..f09b3428e3f15c --- /dev/null +++ b/src/tests/JIT/opt/JitMinOpts/Regression/GitHub_42719_opt.ilproj @@ -0,0 +1,9 @@ + + + Exe + True + + + + + From 79996eef01a366fafefcaa6bb8e3fcd5d12064e1 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Thu, 26 Nov 2020 04:02:14 +0300 Subject: [PATCH 4/5] Update codegenxarch.cpp --- src/coreclr/jit/codegenxarch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/codegenxarch.cpp b/src/coreclr/jit/codegenxarch.cpp index 17ef27b2633f5d..daa655741c6e47 100644 --- a/src/coreclr/jit/codegenxarch.cpp +++ b/src/coreclr/jit/codegenxarch.cpp @@ -5958,7 +5958,7 @@ void CodeGen::genCompareInt(GenTree* treeNode) if (compiler->opts.OptimizationEnabled()) { emitAttr op1Size = emitActualTypeSize(op1->TypeGet()); - assert(static_cast(op1Size) >= 32); + assert((int)op1Size >= 4); // Optimize "x<0" and "x>=0" to "x>>31" if "x" is not a jump condition and in a reg. // Morph/Lowering are responsible to rotate "00" so we won't handle it here. From 994989f0c06b52ec31cb75a858239d3e5ff48950 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 28 Nov 2020 14:14:40 +0300 Subject: [PATCH 5/5] enable test --- .../Microsoft.CSharp/tests/IntegerBinaryOperationTests.cs | 1 - 1 file changed, 1 deletion(-) diff --git a/src/libraries/Microsoft.CSharp/tests/IntegerBinaryOperationTests.cs b/src/libraries/Microsoft.CSharp/tests/IntegerBinaryOperationTests.cs index fe21b494228a29..8888f6ce06723c 100644 --- a/src/libraries/Microsoft.CSharp/tests/IntegerBinaryOperationTests.cs +++ b/src/libraries/Microsoft.CSharp/tests/IntegerBinaryOperationTests.cs @@ -498,7 +498,6 @@ public void RuntimeExpression(object x, object y, ExpressionType type, object re [MemberData(nameof(UInt64TestNotEquals))] [MemberData(nameof(UInt64TestSubtractions))] [ActiveIssue("https://github.com/dotnet/runtime/issues/26798", TargetFrameworkMonikers.NetFramework)] - [SkipOnCoreClr("https://github.com/dotnet/runtime/issues/42719", RuntimeConfiguration.Checked)] public void ConstantExpressions(object x, object y, ExpressionType type, object result, bool shouldSucceedChecked) { var callsite = GetBinaryOperationCallSite(type, false, true, true);