Fix for a bug in fgMorphRecognizeBoxNullable.
authorEugene Rozenfeld <erozen@microsoft.com>
Tue, 20 Jun 2017 23:46:02 +0000 (16:46 -0700)
committerEugene Rozenfeld <erozen@microsoft.com>
Wed, 21 Jun 2017 05:35:56 +0000 (22:35 -0700)
fgMorphRecognizeBoxNullable doesn't work correctly when called during re-morphing. In particular it can't handle late args in the call to CORINFO_HELP_BOX_NULLABLE. The test case results in several asserts (the first one in lsra) followed by an infinite loop in the jit.

The reason the optimization is not performed during global morph is that only patterns with GT_EQ and GT_NE are handled. Some versions of csc generate

...
ldnull
cgt.un
ret

for

this C# line

null != this.value

so we get GT_GT instead.

The fix has two parts:

1. Don't attempt to perform the optimization when called during re-morph and the struct parameter is a late arg.
2. Call fgMorphRecognizeBoxNullable for GT_GT nodes with GTF_UNSIGNED set. This allows the optimization to fire during global morph.

src/jit/morph.cpp
tests/src/JIT/Regression/JitBlue/GitHub_12392/GitHub_12392.cs [new file with mode: 0644]
tests/src/JIT/Regression/JitBlue/GitHub_12392/GitHub_12392.csproj [new file with mode: 0644]

index 18c570b..23fbd5b 100644 (file)
@@ -10585,6 +10585,43 @@ GenTree* Compiler::fgMorphForRegisterFP(GenTree* tree)
     return tree;
 }
 
+//--------------------------------------------------------------------------------------------------------------
+// fgMorphRecognizeBoxNullable:
+//   Recognize this pattern:
+//
+//   stmtExpr  void  (IL 0x000...  ???)
+//     return    int
+//             CNS_INT     ref    null
+//         EQ/NE/GT        int
+//             CALL help ref    HELPER.CORINFO_HELP_BOX_NULLABLE
+//                 CNS_INT(h)  long   0x7fed96836c8 class
+//                 ADDR      byref
+//                     FIELD struct value
+//                         LCL_VAR ref V00 this
+//
+//   which comes from this code:
+//
+//      return this.value==null;
+//
+//   and transform it into
+//
+//   stmtExpr  void  (IL 0x000...  ???)
+//     return    int
+//             CNS_INT     ref    null
+//         EQ/NE/GT        int
+//             IND bool
+//                 ADDR      byref
+//                     FIELD struct value
+//                         LCL_VAR ref V00 this
+//
+// Arguments:
+//       compare - Compare tree to optimize.
+//
+// return value:
+//       A tree that has a call to CORINFO_HELP_BOX_NULLABLE optimized away if the pattern is found;
+//       the original tree otherwise.
+//
+
 GenTree* Compiler::fgMorphRecognizeBoxNullable(GenTree* compare)
 {
     GenTree*     op1 = compare->gtOp.gtOp1;
@@ -10592,26 +10629,6 @@ GenTree* Compiler::fgMorphRecognizeBoxNullable(GenTree* compare)
     GenTree*     opCns;
     GenTreeCall* opCall;
 
-    // recognize this pattern:
-    //
-    // stmtExpr  void  (IL 0x000...  ???)
-    //     return    int
-    //             const     ref    null
-    //         ==        int
-    //             call help ref    HELPER.CORINFO_HELP_BOX_NULLABLE
-    //                 const(h)  long   0x7fed96836c8 class
-    //                 addr      byref
-    //                     ld.lclVar struct V00 arg0
-    //
-    //
-    // which comes from this code (reported by customer as being slow) :
-    //
-    // private static bool IsNull<T>(T arg)
-    // {
-    //    return arg==null;
-    // }
-    //
-
     if (op1->IsCnsIntOrI() && op2->IsHelperCall())
     {
         opCns  = op1;
@@ -10637,8 +10654,17 @@ GenTree* Compiler::fgMorphRecognizeBoxNullable(GenTree* compare)
         return compare;
     }
 
-    // replace the box with an access of the nullable 'hasValue' field which is at the zero offset
-    GenTree* newOp = gtNewOperNode(GT_IND, TYP_BOOL, opCall->gtCall.gtCallArgs->gtOp.gtOp2->gtOp.gtOp1);
+    // Get the nullable struct argument
+    GenTree* arg = opCall->gtCall.gtCallArgs->gtOp.gtOp2->gtOp.gtOp1;
+
+    // Check for cases that are unsafe to optimize and return the unchanged tree
+    if (arg->IsArgPlaceHolderNode() || arg->IsNothingNode() || ((arg->gtFlags & GTF_LATE_ARG) != 0))
+    {
+        return compare;
+    }
+
+    // Replace the box with an access of the nullable 'hasValue' field which is at the zero offset
+    GenTree* newOp = gtNewOperNode(GT_IND, TYP_BOOL, arg);
 
     if (opCall == op1)
     {
@@ -11496,7 +11522,17 @@ GenTreePtr Compiler::fgMorphSmpOp(GenTreePtr tree, MorphAddrContext* mac)
                         }
                     }
                 }
-                fgMorphRecognizeBoxNullable(tree);
+
+                __fallthrough;
+
+            case GT_GT:
+
+                // Try to optimize away calls to CORINFO_HELP_BOX_NULLABLE for GT_EQ, GT_NE, and unsigned GT_GT.
+                if ((oper != GT_GT) || tree->IsUnsigned())
+                {
+                    fgMorphRecognizeBoxNullable(tree);
+                }
+
                 op1 = tree->gtOp.gtOp1;
                 op2 = tree->gtGetOp2IfPresent();
 
diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_12392/GitHub_12392.cs b/tests/src/JIT/Regression/JitBlue/GitHub_12392/GitHub_12392.cs
new file mode 100644 (file)
index 0000000..23058fd
--- /dev/null
@@ -0,0 +1,108 @@
+// 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.Reflection;
+
+namespace Test
+{
+    // This is a regression test for a bug in fgMorphRecognizeBoxNullable.
+    // See the comment in Setting<T>.HasValue for details.
+    class Program
+    {
+
+        static int Main(string[] args)
+        {
+            Test t = new Test();
+            if (!t.TestMethod())
+            {
+                Console.WriteLine("SUCCESS");
+                return 100;
+            }
+            else
+            {
+                Console.WriteLine("FAILURE");
+                return 0;
+            }
+        }
+    }
+
+    class Test
+    {
+        Setting<bool?> supportInteractive = Setting.ForBool(null);
+
+        public bool TestMethod()
+        {
+            return this.supportInteractive.HasValue;
+        }
+    }
+
+    public class Setting<T>
+    {
+        Setting()
+        {
+        }
+
+        public Setting(T value)
+        {
+            this.value = value;
+        }
+
+        public bool HasValue
+        {
+            get
+            {
+                if (this.value != null)
+                {
+                    Type t = this.value.GetType();
+                    if (t.IsGenericType)
+                    {
+                        if (t.GetGenericTypeDefinition() == typeof(Nullable<>))
+                        {
+                            PropertyInfo hasValueProperty = t.GetProperty("HasValue");
+                            bool result = (bool)hasValueProperty.GetValue(this.value, null);
+                            return result;
+                        }
+                    }
+                }
+
+                // The bug reproduces when the C# compiler generates
+                //   ldnull
+                //   cgt.un
+                //   ret
+                // for this statement.
+                // The code above this statement is necessary so that some assertions are
+                // propagated and the statement gets re-morphed.
+                // The bug in fgMorphRecognizeBoxNullable was that it couldn't deal
+                // with a morphed helper call correctly.
+                return null != this.value;
+            }
+        }
+
+        T value;
+        public T Value
+        {
+            get { return this.value; }
+            set { this.value = value; }
+        }
+    }
+
+    public class Setting
+    {
+        Setting()
+        {
+            ;
+        }
+
+        public static Setting<bool?> ForBool(string parameter)
+        {
+            if (null == parameter)
+            {
+                return new Setting<bool?>(null);
+            }
+            Setting<bool?> setting = new Setting<bool?>(bool.Parse(parameter));
+            return setting;
+        }
+    }
+}
diff --git a/tests/src/JIT/Regression/JitBlue/GitHub_12392/GitHub_12392.csproj b/tests/src/JIT/Regression/JitBlue/GitHub_12392/GitHub_12392.csproj
new file mode 100644 (file)
index 0000000..2cafa4c
--- /dev/null
@@ -0,0 +1,38 @@
+<?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>