Fix VN propagation for binops.

When value numbering processes certain binary operators, it may attempt
to propagate the value number of one operand to the operator itself.
Unfortunately, this propagation was done irrespective of the type of the
operand, and therefore produced incorrect results when the type of the
operand did not match the type of the operator. This change only
performs this sort of propagation only when the types of the operator
and the operand agree.

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

diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp
index c36ae1c..c9bb277 100644 (file)
--- a/src/coreclr/src/jit/valuenum.cpp
+++ b/src/coreclr/src/jit/valuenum.cpp
@@ -1029,9 +1029,9 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
         {
             if (typ != TYP_BYREF) // We don't want/need to optimize a zero byref
             {
-                genTreeOps oper = genTreeOps(func);
-                ValueNum   ZeroVN, OneVN; // We may need to create one of these in the switch below.
-                switch (oper)
+                ValueNum resultVN = NoVN;
+                ValueNum ZeroVN, OneVN; // We may need to create one of these in the switch below.
+                switch (genTreeOps(func))
                 {
                     case GT_ADD:
                         // This identity does not apply for floating point (when x == -0.0)
@@ -1041,11 +1041,11 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                             ZeroVN = VNZeroForType(typ);
                             if (arg0VN == ZeroVN)
                             {
-                                return arg1VN;
+                                resultVN = arg1VN;
                             }
                             else if (arg1VN == ZeroVN)
                             {
-                                return arg0VN;
+                                resultVN = arg0VN;
                             }
                         }
                         break;
@@ -1055,7 +1055,7 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         ZeroVN = VNZeroForType(typ);
                         if (arg1VN == ZeroVN)
                         {
-                            return arg0VN;
+                            resultVN = arg0VN;
                         }
                         break;
 
@@ -1066,11 +1066,11 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         {
                             if (arg0VN == OneVN)
                             {
-                                return arg1VN;
+                                resultVN = arg1VN;
                             }
                             else if (arg1VN == OneVN)
                             {
-                                return arg0VN;
+                                resultVN = arg0VN;
                             }
                         }
 
@@ -1080,11 +1080,11 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                             ZeroVN = VNZeroForType(typ);
                             if (arg0VN == ZeroVN)
                             {
-                                return ZeroVN;
+                                resultVN = ZeroVN;
                             }
                             else if (arg1VN == ZeroVN)
                             {
-                                return ZeroVN;
+                                resultVN = ZeroVN;
                             }
                         }
                         break;
@@ -1097,7 +1097,7 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         {
                             if (arg1VN == OneVN)
                             {
-                                return arg0VN;
+                                resultVN = arg0VN;
                             }
                         }
                         break;
@@ -1109,11 +1109,11 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         ZeroVN = VNZeroForType(typ);
                         if (arg0VN == ZeroVN)
                         {
-                            return arg1VN;
+                            resultVN = arg1VN;
                         }
                         else if (arg1VN == ZeroVN)
                         {
-                            return arg0VN;
+                            resultVN = arg0VN;
                         }
                         break;
 
@@ -1122,11 +1122,11 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         ZeroVN = VNZeroForType(typ);
                         if (arg0VN == ZeroVN)
                         {
-                            return ZeroVN;
+                            resultVN = ZeroVN;
                         }
                         else if (arg1VN == ZeroVN)
                         {
-                            return ZeroVN;
+                            resultVN = ZeroVN;
                         }
                         break;
 
@@ -1142,7 +1142,7 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         ZeroVN = VNZeroForType(typ);
                         if (arg1VN == ZeroVN)
                         {
-                            return arg0VN;
+                            resultVN = arg0VN;
                         }
                         break;
 
@@ -1150,30 +1150,35 @@ ValueNum ValueNumStore::VNForFunc(var_types typ, VNFunc func, ValueNum arg0VN, V
                         // (x == x) => true (unless x is NaN)
                         if (!varTypeIsFloating(TypeOfVN(arg0VN)) && (arg0VN != NoVN) && (arg0VN == arg1VN))
                         {
-                            return VNOneForType(typ);
+                            resultVN = VNOneForType(typ);
                         }
                         if ((arg0VN == VNForNull() && IsKnownNonNull(arg1VN)) ||
                             (arg1VN == VNForNull() && IsKnownNonNull(arg0VN)))
                         {
-                            return VNZeroForType(typ);
+                            resultVN = VNZeroForType(typ);
                         }
                         break;
                     case GT_NE:
                         // (x != x) => false (unless x is NaN)
                         if (!varTypeIsFloating(TypeOfVN(arg0VN)) && (arg0VN != NoVN) && (arg0VN == arg1VN))
                         {
-                            return VNZeroForType(typ);
+                            resultVN = VNZeroForType(typ);
                         }
                         if ((arg0VN == VNForNull() && IsKnownNonNull(arg1VN)) ||
                             (arg1VN == VNForNull() && IsKnownNonNull(arg0VN)))
                         {
-                            return VNOneForType(typ);
+                            resultVN = VNOneForType(typ);
                         }
                         break;
 
                     default:
                         break;
                 }
+
+                if ((resultVN != NoVN) && (TypeOfVN(resultVN) == typ))
+                {
+                    return resultVN;
+                }
             }
         }
         else // must be a VNF_ function

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_278369/DevDiv_278369.il b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_278369/DevDiv_278369.il
new file mode 100644 (file)
index 0000000..cf07778
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_278369/DevDiv_278369.il
@@ -0,0 +1,58 @@
+// 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.
+
+// This test checks that value numbering does not incorrectly propagate the value number for an operand to its
+// consuming operator unless the type of the operand and the operator agree.
+//
+// In particular, in the body of method N, the optimizer will fold the two `ceq` conditions together using a logical
+// or of type int32. Value numbering will then determine that one operand to the logical or is a constant `0` (namely,
+// the result of `ldloc.0` and attempt to propagate the value number of the logical or's other operand to the logical
+// or itself. This should not succeed, as the type of the logical or is int32 and the type of its non-zero operand is
+// `ref`.
+
+.assembly extern mscorlib {}
+
+.assembly r {}
+
+.class private auto ansi beforefieldinit C extends [mscorlib]System.Object
+{
+    .method private hidebysig static int32 N(object o) cil managed noinlining
+    {
+        .locals (int32)
+
+        ldc.i4.0
+        stloc.0
+        ldarg.0
+        isinst C
+        ldnull
+        ceq
+        brfalse.s label
+        ldloc.0
+        ldc.i4.0
+        ceq
+        brfalse.s label
+        ldc.i4.s 0
+        ret
+
+label:
+        ldc.i4 100
+        ret
+    }
+
+    .method private hidebysig static int32 Main(string[] args) cil managed
+    {
+        .entrypoint
+
+        newobj instance void C::.ctor()
+        call int32 C::N(object)
+        ret
+    }
+
+    .method public hidebysig specialname rtspecialname instance void  .ctor() cil managed
+    {
+        ldarg.0
+        call instance void [mscorlib]System.Object::.ctor()
+        ret
+    }
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_278369/DevDiv_278369.ilproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_278369/DevDiv_278369.ilproj
new file mode 100644 (file)
index 0000000..deebeec
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/DevDiv_278369/DevDiv_278369.ilproj
@@ -0,0 +1,41 @@
+<?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>
+    <AppDesignerFolder>Properties</AppDesignerFolder>
+    <FileAlignment>512</FileAlignment>
+    <ProjectTypeGuids>{786C830F-07A1-408B-BD7F-6EE04809D6DB};{FAE04EC0-301F-11D3-BF4B-00C04F79EFBC}</ProjectTypeGuids>
+    <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT  .0\UITestExtensionPackages</ReferencePath>
+    <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
+    <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
+  </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>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="DevDiv_278369.il" />
+  </ItemGroup>
+  <ItemGroup>
+    <Service Include="{82A7F48D-3B50-4B1E-B82E-3ADA8210C358}" />
+  </ItemGroup>
+  <Import Project="$([MSBuild]::GetDirectoryNameOfFileAbove($(MSBuildThisFileDirectory), dir.targets))\dir.targets" />
+  <PropertyGroup Condition=" '$(MsBuildProjectDirOverride)' != '' ">
+  </PropertyGroup> 
+</Project>