Fix constant propagation with nested structs

Fixes dotnet/coreclr#18672
Test case is here: JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs

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

diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp
index ddc1a0e..b090b80 100644 (file)
--- a/src/coreclr/src/jit/valuenum.cpp
+++ b/src/coreclr/src/jit/valuenum.cpp
@@ -2235,7 +2235,6 @@ TailCall:
     }
     else
     {
-
         // Give up if we've run out of budget.
         if (--(*pBudget) <= 0)
         {
@@ -6082,8 +6081,8 @@ ValueNum Compiler::fgMemoryVNForLoopSideEffects(MemoryKind  memoryKind,
     assert(nonLoopPred != nullptr);
     // What is its memory post-state?
     ValueNum newMemoryVN = GetMemoryPerSsaData(nonLoopPred->bbMemorySsaNumOut[memoryKind])->m_vnPair.GetLiberal();
-    assert(newMemoryVN !=
-           ValueNumStore::NoVN); // We must have processed the single non-loop pred before reaching the loop entry.
+    assert(newMemoryVN != ValueNumStore::NoVN); // We must have processed the single non-loop pred before reaching the
+                                                // loop entry.
 
 #ifdef DEBUG
     if (verbose)
@@ -6352,10 +6351,6 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
 {
     GenTree* lhs = tree->gtGetOp1();
     GenTree* rhs = tree->gtGetOp2();
-#ifdef DEBUG
-    // Sometimes we query the memory ssa map in an assertion, and need a dummy location for the ignored result.
-    unsigned memorySsaNum;
-#endif
 
     if (tree->OperIsInitBlkOp())
     {
@@ -6366,7 +6361,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
         {
             assert(lclVarTree->gtFlags & GTF_VAR_DEF);
             // Should not have been recorded as updating the GC heap.
-            assert(!GetMemorySsaMap(GcHeap)->Lookup(tree, &memorySsaNum));
+            assert(!GetMemorySsaMap(GcHeap)->Lookup(tree));
 
             unsigned lclNum = lclVarTree->GetLclNum();
 
@@ -6375,7 +6370,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
             if (lvaInSsa(lclNum))
             {
                 // Should not have been recorded as updating ByrefExposed.
-                assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree, &memorySsaNum));
+                assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree));
 
                 unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);
 
@@ -6435,7 +6430,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
         if (tree->DefinesLocal(this, &lclVarTree, &isEntire))
         {
             // Should not have been recorded as updating the GC heap.
-            assert(!GetMemorySsaMap(GcHeap)->Lookup(tree, &memorySsaNum));
+            assert(!GetMemorySsaMap(GcHeap)->Lookup(tree));
 
             unsigned      lhsLclNum = lclVarTree->GetLclNum();
             FieldSeqNode* lhsFldSeq = nullptr;
@@ -6443,12 +6438,11 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
             if (lvaInSsa(lhsLclNum))
             {
                 // Should not have been recorded as updating ByrefExposed.
-                assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree, &memorySsaNum));
+                assert(!GetMemorySsaMap(ByrefExposed)->Lookup(tree));
 
                 unsigned lclDefSsaNum = GetSsaNumForLocalVarDef(lclVarTree);
 
-                if (lhs->IsLocalExpr(this, &lclVarTree, &lhsFldSeq) ||
-                    (lhs->OperIsBlk() && (lhs->AsBlk()->gtBlkSize == lvaLclSize(lhsLclNum))))
+                if (lhs->IsLocalExpr(this, &lclVarTree, &lhsFldSeq))
                 {
                     noway_assert(lclVarTree->gtLclNum == lhsLclNum);
                 }
@@ -6599,12 +6593,15 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree)
                 }
                 else if (lhsFldSeq != nullptr && isEntire)
                 {
-                    // This can occur in for structs with one field, itself of a struct type.
-                    // We won't promote these.
-                    // TODO-Cleanup: decide what exactly to do about this.
-                    // Always treat them as maps, making them use/def, or reconstitute the
-                    // map view here?
-                    isNewUniq = true;
+                    // This can occur for structs with one field, itself of a struct type.
+                    // We are assigning the one field and it is also the entire enclosing struct.
+                    //
+                    // Use an unique value number for the old map, as this is an an entire assignment
+                    // and we won't have any other values in the map
+                    ValueNumPair oldMap;
+                    oldMap.SetBoth(vnStore->VNForExpr(compCurBB, lclVarTree->TypeGet()));
+                    rhsVNPair = vnStore->VNPairApplySelectorsAssign(oldMap, lhsFldSeq, rhsVNPair, lclVarTree->TypeGet(),
+                                                                    compCurBB);
                 }
                 else if (!isNewUniq)
                 {
@@ -8479,8 +8476,8 @@ void Compiler::fgValueNumberHelperCallFunc(GenTreeCall* call, VNFunc vnf, ValueN
         // Add the accumulated exceptions.
         call->gtVNPair = vnStore->VNPWithExc(call->gtVNPair, vnpExc);
     }
-    assert(args == nullptr ||
-           generateUniqueVN); // All arguments should be processed or we generate unique VN and do not care.
+    assert(args == nullptr || generateUniqueVN); // All arguments should be processed or we generate unique VN and do
+                                                 // not care.
 }
 
 void Compiler::fgValueNumberCall(GenTreeCall* call)

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs
new file mode 100644 (file)
index 0000000..51695fd
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.cs
@@ -0,0 +1,69 @@
+// 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;
+
+struct S4
+{
+    int FI1;
+}
+
+struct S24
+{
+    public int FI2;
+    public ulong FU3;
+    public S4 FS4;
+    public S24(int i, S4 s): this()
+    {
+        FI2 = i;
+        FS4 = s;
+    }
+}
+
+// A 24-byte struct that simply wraps S24
+struct S24W
+{
+    public S24 FS24;
+    public S24W(S24 s): this()
+    {
+        FS24 = s;
+    }
+}
+
+public class Program
+{
+    public static int Main()
+    {
+        S4    s4   = new S4();
+        S24   s24  = new S24(1, s4);
+        S24W  s24W = new S24W(s24);
+        S24   s24X = s24;
+
+        int   fI2  = s24.FI2;
+        int   fI2W = s24W.FS24.FI2;
+        int   fI2X = s24X.FI2;
+
+        M();
+
+        if ((fI2 == 1) && (fI2W == 1) && (fI2X == 1))
+        {
+            System.Console.WriteLine("Passed");
+            return 100;
+        }
+        else
+        {
+            // Before the fix we would fail with:
+            //
+            System.Console.WriteLine(fI2);   // 1 in debug, 1 in release  OK
+            System.Console.WriteLine(fI2W);  // 1 in debug, 0 in release  BAD
+            System.Console.WriteLine(fI2X);  // 1 in debug, 1 in release  OK
+
+            System.Console.WriteLine("Failed");
+            return 101;
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void M(){}
+}

diff --git a/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.csproj b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.csproj
new file mode 100644 (file)
index 0000000..685e3e8
--- /dev/null
+++ b/src/coreclr/tests/src/JIT/Regression/JitBlue/GitHub_18672/GitHub_18672.csproj
@@ -0,0 +1,39 @@
+<?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\11.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>
+  <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="GitHub_18672.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>
\ No newline at end of file