Fix CSE side effect and definition extraction

Author: mikedn

src/jit/compiler.h

@@ -5740,12 +5740,6 @@ class Compiler
     bool optCSE_canSwap(GenTree* firstNode, GenTree* secondNode);
     bool optCSE_canSwap(GenTree* tree);
 
-    static fgWalkPostFn optPropagateNonCSE;
-    static fgWalkPreFn  optHasNonCSEChild;
-
-    static fgWalkPreFn optUnmarkCSEs;
-    static fgWalkPreFn optHasCSEdefWithSideeffect;
-
     static int __cdecl optCSEcostCmpEx(const void* op1, const void* op2);
     static int __cdecl optCSEcostCmpSz(const void* op1, const void* op2);
 
@@ -5773,7 +5767,6 @@ class Compiler
     void     optValnumCSE_DataFlow();
     void     optValnumCSE_Availablity();
     void     optValnumCSE_Heuristic();
-    bool optValnumCSE_UnmarkCSEs(GenTree* deadTree, GenTree** wbKeepList);
 
 #endif // FEATURE_VALNUM_CSE
 

src/jit/gentree.cpp

@@ -15161,52 +15161,105 @@ void Compiler::gtExtractSideEffList(GenTree*  expr,
         {
             GenTree* node = *use;
 
-            if (!m_compiler->gtTreeHasSideEffects(node, m_flags))
-            {
-                return Compiler::WALK_SKIP_SUBTREES;
-            }
+            bool treeHasSideEffects = m_compiler->gtTreeHasSideEffects(node, m_flags);
 
-            if (m_compiler->gtNodeHasSideEffects(node, m_flags))
+            if (treeHasSideEffects)
             {
-                m_sideEffects.Push(node);
-                return Compiler::WALK_SKIP_SUBTREES;
-            }
+                if (m_compiler->gtNodeHasSideEffects(node, m_flags))
+                {
+                    m_sideEffects.Push(node);
+                    return Compiler::WALK_SKIP_SUBTREES;
+                }
 
-            // TODO-Cleanup: These have GTF_ASG set but for some reason gtNodeHasSideEffects ignores
-            // them. See the related gtNodeHasSideEffects comment as well.
-            // Also, these nodes must always be preserved, no matter what side effect flags are passed
-            // in. But then it should never be the case that gtExtractSideEffList gets called without
-            // specifying GTF_ASG so there doesn't seem to be any reason to be inconsistent with
-            // gtNodeHasSideEffects and make this check unconditionally.
-            if (node->OperIsAtomicOp())
-            {
-                m_sideEffects.Push(node);
-                return Compiler::WALK_SKIP_SUBTREES;
-            }
+                // TODO-Cleanup: These have GTF_ASG set but for some reason gtNodeHasSideEffects ignores
+                // them. See the related gtNodeHasSideEffects comment as well.
+                // Also, these nodes must always be preserved, no matter what side effect flags are passed
+                // in. But then it should never be the case that gtExtractSideEffList gets called without
+                // specifying GTF_ASG so there doesn't seem to be any reason to be inconsistent with
+                // gtNodeHasSideEffects and make this check unconditionally.
+                if (node->OperIsAtomicOp())
+                {
+                    m_sideEffects.Push(node);
+                    return Compiler::WALK_SKIP_SUBTREES;
+                }
 
-            if ((m_flags & GTF_EXCEPT) != 0)
-            {
-                // Special case - GT_ADDR of GT_IND nodes of TYP_STRUCT have to be kept together.
-                if (node->OperIs(GT_ADDR) && node->gtGetOp1()->OperIsIndir() &&
-                    (node->gtGetOp1()->TypeGet() == TYP_STRUCT))
+                if ((m_flags & GTF_EXCEPT) != 0)
                 {
-#ifdef DEBUG
-                    if (m_compiler->verbose)
+                    // Special case - GT_ADDR of GT_IND nodes of TYP_STRUCT have to be kept together.
+                    if (node->OperIs(GT_ADDR) && node->gtGetOp1()->OperIsIndir() &&
+                        (node->gtGetOp1()->TypeGet() == TYP_STRUCT))
                     {
-                        printf("Keep the GT_ADDR and GT_IND together:\n");
-                    }
+#ifdef DEBUG
+                        if (m_compiler->verbose)
+                        {
+                            printf("Keep the GT_ADDR and GT_IND together:\n");
+                        }
 #endif
+                        m_sideEffects.Push(node);
+                        return Compiler::WALK_SKIP_SUBTREES;
+                    }
+                }
+
+                // Generally all GT_CALL nodes are considered to have side-effects.
+                // So if we get here it must be a helper call that we decided it does
+                // not have side effects that we needed to keep.
+                assert(!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER));
+            }
+
+            if ((m_flags & GTF_IS_IN_CSE) != 0)
+            {
+                // If we're doing CSE then we also need to unmark CSE nodes. This will fail for CSE defs,
+                // those need to be extracted as if they're side effects.
+                if (!UnmarkCSE(node))
+                {
                     m_sideEffects.Push(node);
                     return Compiler::WALK_SKIP_SUBTREES;
                 }
+
+                // This node is going away, decrement the ref count if it's a GT_LCL_VAR.
+                // TODO-Cleanup: This seems incorrect, the ref count should also be decremented for
+                // GT_LCL_FLD nodes. This won't be needed anymore if the JIT stops doing incremental
+                // ref count updates.
+                if (node->OperIs(GT_LCL_VAR))
+                {
+                    unsigned lclNum = node->AsLclVar()->GetLclNum();
+                    assert(lclNum < m_compiler->lvaCount);
+                    LclVarDsc* varDsc = &m_compiler->lvaTable[lclNum];
+                    assert(m_compiler->optCSEweight <= BB_MAX_WEIGHT);
+                    varDsc->decRefCnts(m_compiler->optCSEweight, m_compiler);
+                }
+
+                // The existence of CSE defs and uses is not propagated up the tree like side
+                // effects are. We need to continue visiting the tree as if it has side effects.
+                treeHasSideEffects = true;
             }
 
-            // Generally all GT_CALL nodes are considered to have side-effects.
-            // So if we get here it must be a helper call that we decided it does
-            // not have side effects that we needed to keep.
-            assert(!node->OperIs(GT_CALL) || (node->AsCall()->gtCallType == CT_HELPER));
+            return treeHasSideEffects ? Compiler::WALK_CONTINUE : Compiler::WALK_SKIP_SUBTREES;
+        }
+
+    private:
+        bool UnmarkCSE(GenTree* node)
+        {
+            assert(m_compiler->optValnumCSE_phase);
 
-            return Compiler::WALK_CONTINUE;
+            if (m_compiler->optUnmarkCSE(node))
+            {
+                // The call to optUnmarkCSE(node) should have cleared any CSE info.
+                assert(!IS_CSE_INDEX(node->gtCSEnum));
+                return true;
+            }
+            else
+            {
+                assert(IS_CSE_DEF(node->gtCSEnum));
+#ifdef DEBUG
+                if (m_compiler->verbose)
+                {
+                    printf("Preserving the CSE def #%02d at ", GET_CSE_INDEX(node->gtCSEnum));
+                    m_compiler->printTreeID(node);
+                }
+#endif
+                return false;
+            }
         }
     };
 

src/jit/gentree.h

@@ -706,7 +706,6 @@ struct GenTree
 // The only requirement of this flag is that it not overlap any of the
 // side-effect flags. The actual bit used is otherwise arbitrary.
 #define GTF_IS_IN_CSE GTF_BOOLEAN
-#define GTF_PERSISTENT_SIDE_EFFECTS_IN_CSE (GTF_ASG | GTF_CALL | GTF_IS_IN_CSE)
 
 // Can any side-effects be observed externally, say by a caller method?
 // For assignments, only assignments to global memory can be observed