From 7f94e6ec36ec2a41b06195ee1ed7f89ce647cb57 Mon Sep 17 00:00:00 2001
From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com>
Date: Thu, 31 Mar 2022 23:39:31 +0300
Subject: [PATCH] Account for global stores in hoisting properly (#67213)

* Take HWI stores into account in hoisting

* Also fix ordinary stores while we're at it

The contract for whether the LHS of a global ASG(IND, ...)
should be marked with GLOB_REF is not clearly specified.

* Add tests
---
 src/coreclr/jit/optimizer.cpp              | 28 +++++----
 src/tests/JIT/opt/Hoisting/Hoisting.cs     | 66 ++++++++++++++++++++++
 src/tests/JIT/opt/Hoisting/Hoisting.csproj | 13 +++++
 3 files changed, 95 insertions(+), 12 deletions(-)
 create mode 100644 src/tests/JIT/opt/Hoisting/Hoisting.cs
 create mode 100644 src/tests/JIT/opt/Hoisting/Hoisting.csproj

diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp
index 8815d1ae404..31c2eb672da 100644
--- a/src/coreclr/jit/optimizer.cpp
+++ b/src/coreclr/jit/optimizer.cpp
@@ -6900,23 +6900,27 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack<BasicBlock*>* blo
                         }
                     }
                 }
-                else if (tree->OperIs(GT_ASG))
+                else if (tree->OperRequiresAsgFlag())
                 {
-                    // If the LHS of the assignment has a global reference, then assume it's a global side effect.
-                    GenTree* lhs = tree->AsOp()->gtOp1;
-                    if (lhs->gtFlags & GTF_GLOB_REF)
+                    // Assume all stores except "ASG(non-addr-exposed LCL, ...)" are globally visible.
+                    GenTreeLclVarCommon* lclNode;
+                    bool                 isGloballyVisibleStore;
+                    if (tree->OperIs(GT_ASG) && tree->DefinesLocal(m_compiler, &lclNode))
                     {
+                        isGloballyVisibleStore = m_compiler->lvaGetDesc(lclNode)->IsAddressExposed();
+                    }
+                    else
+                    {
+                        isGloballyVisibleStore = true;
+                    }
+
+                    if (isGloballyVisibleStore)
+                    {
+                        INDEBUG(failReason = "store to globally visible memory");
+                        treeIsHoistable    = false;
                         m_beforeSideEffect = false;
                     }
                 }
-                else if (tree->OperIs(GT_XADD, GT_XORR, GT_XAND, GT_XCHG, GT_LOCKADD, GT_CMPXCHG, GT_MEMORYBARRIER))
-                {
-                    // If this node is a MEMORYBARRIER or an Atomic operation
-                    // then don't hoist and stop any further hoisting after this node
-                    INDEBUG(failReason = "atomic op or memory barrier";)
-                    treeIsHoistable    = false;
-                    m_beforeSideEffect = false;
-                }
             }
 
             // If this 'tree' is hoistable then we return and the caller will
diff --git a/src/tests/JIT/opt/Hoisting/Hoisting.cs b/src/tests/JIT/opt/Hoisting/Hoisting.cs
new file mode 100644
index 00000000000..3bb61b9fe6d
--- /dev/null
+++ b/src/tests/JIT/opt/Hoisting/Hoisting.cs
@@ -0,0 +1,66 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+using System;
+using System.Runtime.CompilerServices;
+using System.Runtime.Intrinsics;
+using System.Runtime.Intrinsics.X86;
+using System.Runtime.Intrinsics.Arm;
+
+unsafe class Hoisting
+{
+    public static int Main()
+    {
+        var p = stackalloc int[4];
+        p[0] = 1;
+        try
+        {
+            ProblemWithHwiStore(p, -1);
+            return 101;
+        }
+        catch (OverflowException)
+        {
+            if (p[0] != 0)
+            {
+                return 102;
+            }
+        }
+
+        try
+        {
+            ProblemWithNormalStore(p, -1);
+            return 103;
+        }
+        catch (OverflowException)
+        {
+            if (p[0] != -1)
+            {
+                return 104;
+            }
+        }
+
+        return 100;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void ProblemWithHwiStore(int* p, int b)
+    {
+        // Make sure we don't hoist the checked cast.
+        for (int i = 0; i < 10; i++)
+        {
+            Vector128.Store(Vector128<int>.Zero, p);
+            *p = (int)checked((uint)b);
+        }
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    private static void ProblemWithNormalStore(int* p, int b)
+    {
+        // Make sure we don't hoist the checked cast.
+        for (int i = 0; i < 10; i++)
+        {
+            *p = b;
+            *p = (int)checked((uint)b);
+        }
+    }
+}
diff --git a/src/tests/JIT/opt/Hoisting/Hoisting.csproj b/src/tests/JIT/opt/Hoisting/Hoisting.csproj
new file mode 100644
index 00000000000..de62e29fa3e
--- /dev/null
+++ b/src/tests/JIT/opt/Hoisting/Hoisting.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+    <AllowUnsafeBlocks>true</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>PdbOnly</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>
-- 
GitLab