Fix dummy OBJ/BLK/IND nodes. (#39824)

* Add a repro.

* Add a helper function `fgTryRemoveNonLocal`.

* Fix the issue.

* extract `gtChangeOperToNullCheck`.

* update comments.

diff --git a/src/coreclr/src/jit/compiler.cpp b/src/coreclr/src/jit/compiler.cpp
index cb129b3..4e1140e 100644 (file)
--- a/src/coreclr/src/jit/compiler.cpp
+++ b/src/coreclr/src/jit/compiler.cpp
@@ -9285,3 +9285,19 @@ bool Compiler::lvaIsOSRLocal(unsigned varNum)
 
     return false;
 }
+
+//------------------------------------------------------------------------------
+// gtChangeOperToNullCheck: helper to change tree oper to a NULLCHECK.
+//
+// Arguments:
+//    tree       - the node to change;
+//    basicBlock - basic block of the node.
+//
+void Compiler::gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block)
+{
+    assert(tree->OperIs(GT_FIELD, GT_IND, GT_OBJ, GT_BLK, GT_DYN_BLK));
+    tree->ChangeOper(GT_NULLCHECK);
+    tree->ChangeType(TYP_BYTE);
+    block->bbFlags |= BBF_HAS_NULLCHECK;
+    optMethodFlags |= OMF_HAS_NULLCHECK;
+}

diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h
index 011a2af..183e84c 100644 (file)
--- a/src/coreclr/src/jit/compiler.h
+++ b/src/coreclr/src/jit/compiler.h
@@ -2701,6 +2701,8 @@ public:
 
     GenTree* gtNewNullCheck(GenTree* addr, BasicBlock* basicBlock);
 
+    void gtChangeOperToNullCheck(GenTree* tree, BasicBlock* block);
+
     GenTreeArgList* gtNewArgList(GenTree* op);
     GenTreeArgList* gtNewArgList(GenTree* op1, GenTree* op2);
     GenTreeArgList* gtNewArgList(GenTree* op1, GenTree* op2, GenTree* op3);
@@ -4633,6 +4635,8 @@ public:
 
     void fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALARG_TP volatileVars);
 
+    bool fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange);
+
     bool fgRemoveDeadStore(GenTree**        pTree,
                            LclVarDsc*       varDsc,
                            VARSET_VALARG_TP life,

diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp
index ca5c6d4..2e0e96a 100644 (file)
--- a/src/coreclr/src/jit/gentree.cpp
+++ b/src/coreclr/src/jit/gentree.cpp
@@ -13782,16 +13782,13 @@ GenTree* Compiler::gtTryRemoveBoxUpstreamEffects(GenTree* op, BoxRemovalOptions
         // For struct types read the first byte of the
         // source struct; there's no need to read the
         // entire thing, and no place to put it.
-        assert(copySrc->gtOper == GT_OBJ || copySrc->gtOper == GT_IND || copySrc->gtOper == GT_FIELD);
+        assert(copySrc->OperIs(GT_OBJ, GT_IND, GT_FIELD));
         copyStmt->SetRootNode(copySrc);
 
         if (options == BR_REMOVE_AND_NARROW || options == BR_REMOVE_AND_NARROW_WANT_TYPE_HANDLE)
         {
             JITDUMP(" to read first byte of struct via modified [%06u]\n", dspTreeID(copySrc));
-            copySrc->ChangeOper(GT_NULLCHECK);
-            copySrc->gtType = TYP_BYTE;
-            compCurBB->bbFlags |= BBF_HAS_NULLCHECK;
-            optMethodFlags |= OMF_HAS_NULLCHECK;
+            gtChangeOperToNullCheck(copySrc, compCurBB);
         }
         else
         {
@@ -15970,6 +15967,11 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
                 if (m_compiler->gtNodeHasSideEffects(node, m_flags))
                 {
                     m_sideEffects.Push(node);
+                    if (node->OperIsBlk() && !node->OperIsStoreBlk())
+                    {
+                        JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
+                        m_compiler->gtChangeOperToNullCheck(node, m_compiler->compCurBB);
+                    }
                     return Compiler::WALK_SKIP_SUBTREES;
                 }
 

diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp
index 9bb5315..d111158 100644 (file)
--- a/src/coreclr/src/jit/importer.cpp
+++ b/src/coreclr/src/jit/importer.cpp
@@ -13247,10 +13247,7 @@ void Compiler::impImportBlockCode(BasicBlock* block)
                             // via an underlying address, just null check the address.
                             if (op1->OperIs(GT_FIELD, GT_IND, GT_OBJ))
                             {
-                                op1->ChangeOper(GT_NULLCHECK);
-                                block->bbFlags |= BBF_HAS_NULLCHECK;
-                                optMethodFlags |= OMF_HAS_NULLCHECK;
-                                op1->gtType = TYP_BYTE;
+                                gtChangeOperToNullCheck(op1, block);
                             }
                             else
                             {

diff --git a/src/coreclr/src/jit/liveness.cpp b/src/coreclr/src/jit/liveness.cpp
index feb8f23..504a6c0 100644 (file)
--- a/src/coreclr/src/jit/liveness.cpp
+++ b/src/coreclr/src/jit/liveness.cpp
@@ -2109,40 +2109,77 @@ void Compiler::fgComputeLifeLIR(VARSET_TP& life, BasicBlock* block, VARSET_VALAR
                 break;
 
             case GT_NOP:
+            {
                 // NOTE: we need to keep some NOPs around because they are referenced by calls. See the dead store
                 // removal code above (case GT_STORE_LCL_VAR) for more explanation.
                 if ((node->gtFlags & GTF_ORDER_SIDEEFF) != 0)
                 {
                     break;
                 }
-                __fallthrough;
+                fgTryRemoveNonLocal(node, &blockRange);
+            }
+            break;
 
-            default:
-                assert(!node->OperIsLocal());
-                if (!node->IsValue() || node->IsUnusedValue())
+            case GT_BLK:
+            case GT_OBJ:
+            case GT_DYN_BLK:
+            {
+                bool removed = fgTryRemoveNonLocal(node, &blockRange);
+                if (!removed && node->IsUnusedValue())
                 {
-                    // We are only interested in avoiding the removal of nodes with direct side-effects
-                    // (as opposed to side effects of their children).
-                    // This default case should never include calls or assignments.
-                    assert(!node->OperRequiresAsgFlag() && !node->OperIs(GT_CALL));
-                    if (!node->gtSetFlags() && !node->OperMayThrow(this))
-                    {
-                        JITDUMP("Removing dead node:\n");
-                        DISPNODE(node);
-
-                        node->VisitOperands([](GenTree* operand) -> GenTree::VisitResult {
-                            operand->SetUnusedValue();
-                            return GenTree::VisitResult::Continue;
-                        });
-
-                        blockRange.Remove(node);
-                    }
+                    // IR doesn't expect dummy uses of `GT_OBJ/BLK/DYN_BLK`.
+                    JITDUMP("Replace an unused OBJ/BLK node [%06d] with a NULLCHECK\n", dspTreeID(node));
+                    gtChangeOperToNullCheck(node, block);
                 }
+            }
+            break;
+
+            default:
+                fgTryRemoveNonLocal(node, &blockRange);
                 break;
         }
     }
 }
 
+//---------------------------------------------------------------------
+// fgTryRemoveNonLocal - try to remove a node if it is unused and has no direct
+//   side effects.
+//
+// Arguments
+//    node       - the non-local node to try;
+//    blockRange - the block range that contains the node.
+//
+// Return value:
+//    None
+//
+// Notes: local nodes are processed independently and are not expected in this function.
+//
+bool Compiler::fgTryRemoveNonLocal(GenTree* node, LIR::Range* blockRange)
+{
+    assert(!node->OperIsLocal());
+    if (!node->IsValue() || node->IsUnusedValue())
+    {
+        // We are only interested in avoiding the removal of nodes with direct side effects
+        // (as opposed to side effects of their children).
+        // This default case should never include calls or assignments.
+        assert(!node->OperRequiresAsgFlag() && !node->OperIs(GT_CALL));
+        if (!node->gtSetFlags() && !node->OperMayThrow(this))
+        {
+            JITDUMP("Removing dead node:\n");
+            DISPNODE(node);
+
+            node->VisitOperands([](GenTree* operand) -> GenTree::VisitResult {
+                operand->SetUnusedValue();
+                return GenTree::VisitResult::Continue;
+            });
+
+            blockRange->Remove(node);
+            return true;
+        }
+    }
+    return false;
+}
+
 // fgRemoveDeadStore - remove a store to a local which has no exposed uses.
 //
 //   pTree          - GenTree** to local, including store-form local or local addr (post-rationalize)

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.cs b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.cs
new file mode 100644 (file)
index 0000000..2264049
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.cs
@@ -0,0 +1,26 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+
+// The test was deleting the hardware intrinsic leaving unconsumed GT_OBJ on top of the stack
+// that was leading to an assert failure.
+
+using System.Runtime.Intrinsics;
+using System.Runtime.Intrinsics.X86;
+using System;
+
+class Runtime_39403
+{ 
+    public static int Main()
+    {
+        if (Sse41.IsSupported)
+        {
+            Vector128<int> left = Vector128.Create(1);
+            Vector128<int> right = Vector128.Create(2);
+            ref var rightRef = ref right;
+            Vector128<int> mask = Vector128.Create(3);
+            Sse41.BlendVariable(left, rightRef, mask);
+        }
+        return 100;
+    }
+}
+

diff --git a/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.csproj b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.csproj
new file mode 100644 (file)
index 0000000..5d49e8d
--- /dev/null
+++ b/src/tests/JIT/Regression/JitBlue/Runtime_39737/Runtime_39737.csproj
@@ -0,0 +1,13 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType />
+    <Optimize>True</Optimize>
+    <AllowUnsafeBlocks>True</AllowUnsafeBlocks>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="$(MSBuildProjectName).cs" />
+  </ItemGroup>
+</Project>