From 70d72b50a501bae4bb561e911dddc5734d6b8943 Mon Sep 17 00:00:00 2001 From: Mike Danes Date: Tue, 13 Dec 2016 14:45:13 +0200 Subject: [PATCH] Fix incorrect compare narrowing in TreeNodeInfoInitCmp TreeNodeInfoInitCmp attempts to eliminate the cast from `cmp(cast(x), icon)` by narrowing the compare to ubyte. This should only happen if the constant fits in a byte so it can be narrowed too, otherwise codegen produces an int sized compare. (or a byte sized compare with a truncated constant if we try to use GTF_RELOP_SMALL). Commit migrated from https://github.com/dotnet/coreclr/commit/5bfdc826d73a2f110bc21f6f20c6927f1a25c6f2 --- src/coreclr/src/jit/lowerxarch.cpp | 53 +++++++++++----------- .../Regression/JitBlue/GitHub_8599/GitHub_8599.cs | 42 +++++++++++++++++ .../JitBlue/GitHub_8599/GitHub_8599.csproj | 46 +++++++++++++++++++ 3 files changed, 114 insertions(+), 27 deletions(-) create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.cs create mode 100644 src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.csproj diff --git a/src/coreclr/src/jit/lowerxarch.cpp b/src/coreclr/src/jit/lowerxarch.cpp index 64283c8..589cef4 100644 --- a/src/coreclr/src/jit/lowerxarch.cpp +++ b/src/coreclr/src/jit/lowerxarch.cpp @@ -3659,17 +3659,6 @@ void Lowering::TreeNodeInfoInitCmp(GenTreePtr tree) // assert(!castOp1->gtOverflowEx()); // Must not be an overflow checking operation - GenTreePtr removeTreeNode = op1; - tree->gtOp.gtOp1 = castOp1; - op1 = castOp1; - castOp1->gtType = TYP_UBYTE; - - // trim down the value if castOp1 is an int constant since its type changed to UBYTE. - if (castOp1Oper == GT_CNS_INT) - { - castOp1->gtIntCon.gtIconVal = (UINT8)castOp1->gtIntCon.gtIconVal; - } - // TODO-Cleanup: we're within "if (CheckImmedAndMakeContained(tree, op2))", so isn't // the following condition always true? if (op2->isContainedIntOrIImmed()) @@ -3677,6 +3666,17 @@ void Lowering::TreeNodeInfoInitCmp(GenTreePtr tree) ssize_t val = (ssize_t)op2->AsIntConCommon()->IconValue(); if (val >= 0 && val <= 255) { + GenTreePtr removeTreeNode = op1; + tree->gtOp.gtOp1 = castOp1; + op1 = castOp1; + castOp1->gtType = TYP_UBYTE; + + // trim down the value if castOp1 is an int constant since its type changed to UBYTE. + if (castOp1Oper == GT_CNS_INT) + { + castOp1->gtIntCon.gtIconVal = (UINT8)castOp1->gtIntCon.gtIconVal; + } + op2->gtType = TYP_UBYTE; tree->gtFlags |= GTF_UNSIGNED; @@ -3687,27 +3687,26 @@ void Lowering::TreeNodeInfoInitCmp(GenTreePtr tree) MakeSrcContained(tree, op1); op1IsMadeContained = true; } - } - } - BlockRange().Remove(removeTreeNode); + BlockRange().Remove(removeTreeNode); - // We've changed the type on op1 to TYP_UBYTE, but we already processed that node. We need to - // go back and mark it byteable. - // TODO-Cleanup: it might be better to move this out of the TreeNodeInfoInit pass to the earlier - // "lower" pass, in which case the byteable check would just fall out. But that is quite - // complex! - TreeNodeInfoInitCheckByteable(op1); + // We've changed the type on op1 to TYP_UBYTE, but we already processed that node. + // We need to go back and mark it byteable. + // TODO-Cleanup: it might be better to move this out of the TreeNodeInfoInit pass to + // the earlier "lower" pass, in which case the byteable check would just fall out. + // But that is quite complex! + TreeNodeInfoInitCheckByteable(op1); #ifdef DEBUG - if (comp->verbose) - { - printf( - "TreeNodeInfoInitCmp: Removing a GT_CAST to TYP_UBYTE and changing castOp1->gtType to " - "TYP_UBYTE\n"); - comp->gtDispTreeRange(BlockRange(), tree); - } + if (comp->verbose) + { + printf("TreeNodeInfoInitCmp: Removing a GT_CAST to TYP_UBYTE and changing " + "castOp1->gtType to TYP_UBYTE\n"); + comp->gtDispTreeRange(BlockRange(), tree); + } #endif + } + } } } diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.cs new file mode 100644 index 0000000..95ff4576 --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.cs @@ -0,0 +1,42 @@ +// Licensed to the .NET Foundation under one or more agreements. +// The .NET Foundation licenses this file to you under the MIT license. +// See the LICENSE file in the project root for more information. + +using System; +using System.Runtime.CompilerServices; + +// TreeNodeInfoInitCmp attempts to eliminate the cast from cmp(cast(x), icon) +// by narrowing the compare to ubyte. This should only happen if the constant fits in +// a byte so it can be narrowed too, otherwise codegen produces an int sized compare. + +class Program +{ + [MethodImpl(MethodImplOptions.NoInlining)] + static int GetValue() => 301; + + static void Escape(ref int x) + { + } + + static int Main() + { + if ((byte)GetValue() > 300) + { + return -1; + } + + int x = GetValue(); + Escape(ref x); + if ((byte)x > 300) + { + return -2; + } + + if ((byte)(GetValue() | 2) > 300) + { + return -3; + } + + return 100; + } +} diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.csproj new file mode 100644 index 0000000..b174dea --- /dev/null +++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_8599/GitHub_8599.csproj @@ -0,0 +1,46 @@ + + + + + Debug + AnyCPU + $(MSBuildProjectName) + 2.0 + {95DFC527-4DC1-495E-97D7-E94EE1F7140D} + Exe + Properties + 512 + {786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC} + $(ProgramFiles)\Common Files\microsoft shared\VSTT\11.0\UITestExtensionPackages + ..\..\ + + 7a9bfb7d + + + + + + + + + False + + + + + True + + + + + + + + + $(JitPackagesConfigFileDirectory)minimal\project.json + $(JitPackagesConfigFileDirectory)minimal\project.lock.json + + + + + -- 2.7.4