From 759e3777aad6e711c6c6171ee738df9f38e877a0 Mon Sep 17 00:00:00 2001
From: Andy Ayers <andya@microsoft.com>
Date: Thu, 11 Apr 2019 16:45:30 -0700
Subject: [PATCH] JIT: fix assert when there are mixed types in Enum.HasFlags
 optimization (#23902)

In some cases the pre-boxed nodes may differ in type. Bail if they don't have
the same stack type, then compute the result using the stack type.

Extended the hasflags test with a case that shows this issue.

Closes #23847
---
 src/jit/gentree.cpp                   | 25 ++++++++++++++++++-------
 tests/src/JIT/opt/Enum/hasflag.cs     | 25 ++++++++++++++++++++++++-
 tests/src/JIT/opt/Enum/hasflag.csproj |  1 -
 3 files changed, 42 insertions(+), 9 deletions(-)

diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp
index 45cc22e..be064c2 100644
--- a/src/jit/gentree.cpp
+++ b/src/jit/gentree.cpp
@@ -12920,7 +12920,8 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp)
         return nullptr;
     }
 
-    GenTree* flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_REMOVE_BUT_NOT_NARROW);
+    // Do likewise with flagOp.
+    GenTree* flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_DONT_REMOVE);
     if (flagVal == nullptr)
     {
         // Note we may fail here if the flag operand comes from
@@ -12929,19 +12930,29 @@ GenTree* Compiler::gtOptimizeEnumHasFlag(GenTree* thisOp, GenTree* flagOp)
         return nullptr;
     }
 
+    // Only proceed when both box sources have the same actual type.
+    // (this rules out long/int mismatches)
+    if (genActualType(thisVal->TypeGet()) != genActualType(flagVal->TypeGet()))
+    {
+        JITDUMP("bailing, pre-boxed values have different types\n");
+        return nullptr;
+    }
+
     // Yes, both boxes can be cleaned up. Optimize.
     JITDUMP("Optimizing call to Enum.HasFlag\n");
 
-    // Undo the boxing of thisOp and prepare to operate directly
-    // on the original enum values.
+    // Undo the boxing of the Ops and prepare to operate directly
+    // on the pre-boxed values.
     thisVal = gtTryRemoveBoxUpstreamEffects(thisOp, BR_REMOVE_BUT_NOT_NARROW);
+    flagVal = gtTryRemoveBoxUpstreamEffects(flagOp, BR_REMOVE_BUT_NOT_NARROW);
 
-    // Our trial removal above should guarantee successful removal here.
+    // Our trial removals above should guarantee successful removals here.
     assert(thisVal != nullptr);
+    assert(flagVal != nullptr);
+    assert(genActualType(thisVal->TypeGet()) == genActualType(flagVal->TypeGet()));
 
-    // We should have a consistent view of the type
-    var_types type = thisVal->TypeGet();
-    assert(type == flagVal->TypeGet());
+    // Type to use for optimized check
+    var_types type = genActualType(thisVal->TypeGet());
 
     // The thisVal and flagVal trees come from earlier statements.
     //
diff --git a/tests/src/JIT/opt/Enum/hasflag.cs b/tests/src/JIT/opt/Enum/hasflag.cs
index 08b418b..0c415ee 100644
--- a/tests/src/JIT/opt/Enum/hasflag.cs
+++ b/tests/src/JIT/opt/Enum/hasflag.cs
@@ -8,6 +8,7 @@
 // except for the case in the try/catch.
 
 using System;
+using System.Runtime.CompilerServices;
 
 enum E
 {
@@ -40,6 +41,12 @@ class EClass
     public E e;
 }
 
+class ShortHolder
+{
+    public ShortHolder(short s) { v = s; }
+    public short v;
+}
+
 class P
 {
     static E[] ArrayOfE = { E.RED, E.BLUE };
@@ -81,6 +88,20 @@ class P
         return e1.HasFlag(e2);
     }
 
+    // Example from GitHub 23847
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    public static bool GitHub23847(E e1, short s)
+    {
+        return GitHub23847Aux(s).HasFlag(e1);
+    }
+
+    // Once this is inlined we end up with a short pre-boxed value
+    public static E GitHub23847Aux(short s)
+    {
+        ShortHolder h = new ShortHolder(s);
+        return (E)h.v;
+    }
+
     public static int Main()
     {
         E e1 = E.RED;
@@ -120,8 +141,10 @@ class P
         G g1 = G.RED;
         bool true9 = ByrefG(ref g1, G.RED);
 
+        bool false10 = GitHub23847(E.RED, 0x100);
+
         bool[] trueResults = {true0, true1, true2, true3, true4, true5, true6, true7, true8, true9};
-        bool[] falseResults = {false0, false1, false2, false3, false4, false5, false6, false7, false8, false9};
+        bool[] falseResults = {false0, false1, false2, false3, false4, false5, false6, false7, false8, false9, false10};
 
         bool resultOk = true;
 
diff --git a/tests/src/JIT/opt/Enum/hasflag.csproj b/tests/src/JIT/opt/Enum/hasflag.csproj
index 42e9f46..a86bec7 100644
--- a/tests/src/JIT/opt/Enum/hasflag.csproj
+++ b/tests/src/JIT/opt/Enum/hasflag.csproj
@@ -14,7 +14,6 @@
     <ReferencePath>$(ProgramFiles)\Common Files\microsoft shared\VSTT	.0\UITestExtensionPackages</ReferencePath>
     <SolutionDir Condition="$(SolutionDir) == '' Or $(SolutionDir) == '*Undefined*'">..\..\</SolutionDir>
     <NuGetPackageImportStamp>7a9bfb7d</NuGetPackageImportStamp>
-    <CLRTestPriority>1</CLRTestPriority>
   </PropertyGroup>
   <!-- Default configurations to help VS understand the configurations -->
   <PropertyGroup Condition=" '$(Configuration)|$(Platform)' == 'Debug|AnyCPU' "></PropertyGroup>
-- 
2.7.4