Properly type morphed NEG nodes (dotnet/coreclr#18837)

* Properly type optimized NEG nodes

When the JIT was morphing trees like '-1 * expr', it would turn the
multiplication into a NEG node with the same type as its right operand.
This is a problem when the right operand was a small type like TYP_UBYTE
because the NEG node always produces a widened result. This could cause
problems when the negation was fed into something that depended on the
type, such as a cast to TYP_UBYTE: here the JIT would conclude that the
cast could be dropped, and end up producing a widened result.

The solution is to give the tree the actual type of the NEG node.

Also add a test for this case and for a similar case of '0 - expr',
which already had a fix.

Fix dotnet/coreclr#18780

* Address PR feedback

* Clarify comment

Commit migrated from https://github.com/dotnet/coreclr/commit/c06c61ab4d8c8d1b212fc7e9a9a130c63d47ed44

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
index f5063b0..53b4090 100644 (file)
--- a/src/coreclr/src/jit/morph.cpp
+++ b/src/coreclr/src/jit/morph.cpp
@@ -12914,9 +12914,11 @@ DONE_MORPHING_CHILDREN:
                 {
                     noway_assert(varTypeIsIntOrI(tree));
 
-                    tree->gtOp.gtOp2 = op2 = gtNewOperNode(GT_NEG, tree->gtType, op2); // The type of the new GT_NEG
-                                                                                       // node should be the same
-                    // as the type of the tree, i.e. tree->gtType.
+                    // The type of the new GT_NEG node cannot just be op2->TypeGet().
+                    // Otherwise we may sign-extend incorrectly in cases where the GT_NEG
+                    // node ends up feeding directly into a cast, for example in
+                    // GT_CAST<ubyte>(GT_SUB(0, s_1.ubyte))
+                    tree->gtOp.gtOp2 = op2 = gtNewOperNode(GT_NEG, genActualType(op2->TypeGet()), op2);
                     fgMorphTreeDone(op2);
 
                     oper = GT_ADD;
@@ -13142,7 +13144,11 @@ DONE_MORPHING_CHILDREN:
                     // if negative negate (min-int does not need negation)
                     if (mult < 0 && mult != SSIZE_T_MIN)
                     {
-                        tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, op1->gtType, op1);
+                        // The type of the new GT_NEG node cannot just be op1->TypeGet().
+                        // Otherwise we may sign-extend incorrectly in cases where the GT_NEG
+                        // node ends up feeding directly a cast, for example in
+                        // GT_CAST<ubyte>(GT_MUL(-1, s_1.ubyte)
+                        tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, genActualType(op1->TypeGet()), op1);
                         fgMorphTreeDone(op1);
                     }
 
@@ -13184,7 +13190,7 @@ DONE_MORPHING_CHILDREN:
                         // if negative negate (min-int does not need negation)
                         if (mult < 0 && mult != SSIZE_T_MIN)
                         {
-                            tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, op1->gtType, op1);
+                            tree->gtOp.gtOp1 = op1 = gtNewOperNode(GT_NEG, genActualType(op1->TypeGet()), op1);
                             fgMorphTreeDone(op1);
                         }
 

diff --git a/src/coreclr/tests/arm/Tests.lst b/src/coreclr/tests/arm/Tests.lst
index 5aa0d21..24a6a39 100644 (file)
--- a/src/coreclr/tests/arm/Tests.lst
+++ b/src/coreclr/tests/arm/Tests.lst
@@ -94828,3 +94828,11 @@ MaxAllowedDurationSeconds=600
 Categories=EXPECTED_PASS
 HostStyle=0
 
+[GitHub_18780.cmd_11914]
+RelativePath=JIT\Regression\JitBlue\GitHub_18780\GitHub_18780\GitHub_18780.cmd
+WorkingDir=JIT\Regression\JitBlue\GitHub_18780\GitHub_18780
+Expected=0
+MaxAllowedDurationSeconds=600
+Categories=EXPECTED_PASS
+HostStyle=0
+

diff --git a/src/coreclr/tests/arm64/Tests.lst b/src/coreclr/tests/arm64/Tests.lst
index bc58f93..126b545 100644 (file)
--- a/src/coreclr/tests/arm64/Tests.lst
+++ b/src/coreclr/tests/arm64/Tests.lst
@@ -94843,3 +94843,11 @@ Expected=0
 MaxAllowedDurationSeconds=600
 Categories=EXPECTED_PASS
 HostStyle=0
+
+[GitHub_18780.cmd_12233]
+RelativePath=JIT\Regression\JitBlue\GitHub_18780\GitHub_18780\GitHub_18780.cmd
+WorkingDir=JIT\Regression\JitBlue\GitHub_18780\GitHub_18780
+Expected=0
+MaxAllowedDurationSeconds=600
+Categories=EXPECTED_PASS
+HostStyle=0

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.cs
new file mode 100644 (file)
index 0000000..1833fe5
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.cs
@@ -0,0 +1,46 @@
+// 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.Runtime.CompilerServices;
+
+public class GitHub_18780
+{
+    public static int Main()
+    {
+        bool ok = true;
+        ok &= M1(0);
+        ok &= M2();
+        ok &= M3();
+        return ok ? 100 : -1;
+    }
+
+    // The multiplication by uint.MaxValue was optimized to a NEG
+    // which was typed as the second operand, giving a byte NEG node.
+    // With x86/ARM32 RyuJIT the cast to ulong would then treat vr13
+    // as a byte instead of a uint and zero extend from 8 bits.
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static bool M1(byte arg2)
+    {
+        byte vr23 = arg2++;
+        return (ulong)(uint.MaxValue * arg2) == uint.MaxValue;
+    }
+
+    // Like above, the -1 multiplication was turned into a byte NEG node.
+    // The byte cast was then removed, but since the NEG byte node still
+    // produces a wide result this meant (byte)val was -1.
+    static byte s_1 = 1;
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static bool M2()
+    {
+        return (byte)(-1 * s_1) == 255;
+    }
+
+    // Exactly the same as above, but this tests the optimization for
+    // transforming (0 - expr) into -expr.
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static bool M3()
+    {
+        return (byte)(0 - s_1) == 255;
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.csproj
new file mode 100644 (file)
index 0000000..95aba99
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18780/GitHub_18780.csproj
@@ -0,0 +1,34 @@
+<?xml version="1.0" encoding="utf-8"?>
+<Project ToolsVersion="12.0" DefaultTargets="Build" xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.props))\dir.props" />
+  <PropertyGroup>
+    <Configuration Condition=" '$(Configuration)' == '' ">Debug</Configuration>
+    <Platform Condition=" '$(Platform)' == '' ">AnyCPU</Platform>
+    <AssemblyName>$(MSBuildProjectName)</AssemblyName>
+    <SchemaVersion>2.0</SchemaVersion>
+    <ProjectGuid>{95DFC527-4DC1-495E-97D7-E94EE1F7140D}</ProjectGuid>
+    <OutputType>Exe</OutputType>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+  </PropertyGroup>
+  <!-- Default configurations to help VS understand the configurations -->
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
+  <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Release|AnyCPU' "></PropertyGroup>
+  <ItemGroup>
+    <CodeAnalysisDependentAssemblyPaths Condition=" '$(VS100COMNTOOLS)' != '' " Include="$(VS100COMNTOOLS)..\IDE\PrivateAssemblies">
+      <Visible>False</Visible>
+    </CodeAnalysisDependentAssemblyPaths>
+  </ItemGroup>
+  <PropertyGroup>
+    <DebugType></DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' "></PropertyGroup>
+</Project>