Address feedback [WIP]

src/coreclr/jit/compiler.h

@@ -2929,6 +2929,8 @@ class Compiler
                               GenTreeFlags GenTreeFlags = GTF_SIDE_EFFECT,
                               bool         ignoreRoot   = false);
 
+    void gtSplitTree(BasicBlock* block, Statement* stmt, GenTree* splitPoint);
+
     // Static fields of struct types (and sometimes the types that those are reduced to) are represented by having the
     // static field contain an object pointer to the boxed struct.  This simplifies the GC implementation...but
     // complicates the JIT somewhat.  This predicate returns "true" iff a node with type "fieldNodeType", representing

src/coreclr/jit/compiler.hpp

@@ -1610,6 +1610,27 @@ inline void GenTree::BashToZeroConst(var_types type)
     }
 }
 
+//------------------------------------------------------------------------
+// BashToLclVar: Bash node to a LCL_VAR.
+//
+// Arguments:
+//    comp   - compiler object
+//    lclNum - the local's number
+//
+// Return Value:
+//    The bashed node.
+//
+inline GenTreeLclVar* GenTree::BashToLclVar(Compiler* comp, unsigned lclNum)
+{
+    LclVarDsc* varDsc = comp->lvaGetDesc(lclNum);
+
+    ChangeOper(GT_LCL_VAR);
+    ChangeType(varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc));
+    AsLclVar()->SetLclNum(lclNum);
+
+    return AsLclVar();
+}
+
 /*****************************************************************************
  *
  * Returns true if the node is of the "ovf" variety, for example, add.ovf.i1.

src/coreclr/jit/gentree.cpp

@@ -16212,6 +16212,31 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S
     return true;
 }
 
+void Compiler::gtSplitTree(BasicBlock* block, Statement* stmt, GenTree* splitPoint)
+{
+    class SideEffectSeparator final : public GenTreeVisitor<SideEffectSeparator>
+    {
+    public:
+        enum
+        {
+            DoPreOrder        = true,
+            UseExecutionOrder = true
+        };
+
+        SideEffectSeparator(Compiler* compiler) : GenTreeVisitor(compiler)
+        {
+            // TODO:
+        }
+
+        fgWalkResult PreOrderVisit(GenTree** use, GenTree* user)
+        {
+            return Compiler::WALK_SKIP_SUBTREES;
+        }
+    };
+    SideEffectSeparator extractor(this);
+    extractor.WalkTree(stmt->GetRootNodePointer(), nullptr);
+}
+
 //------------------------------------------------------------------------
 // gtExtractSideEffList: Extracts side effects from the given expression.
 //

src/coreclr/jit/gentree.h

@@ -1943,8 +1943,8 @@ struct GenTree
 
     template <typename T>
     void BashToConst(T value, var_types type = TYP_UNDEF);
-
     void BashToZeroConst(var_types type);
+    GenTreeLclVar* BashToLclVar(Compiler* comp, unsigned lclNum);
 
 #if NODEBASH_STATS
     static void RecordOperBashing(genTreeOps operOld, genTreeOps operNew);

src/coreclr/jit/lclmorph.cpp

@@ -1154,7 +1154,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
 
             case IndirTransform::BitCast:
                 indir->ChangeOper(GT_BITCAST);
-                lclNode = BashToLclVar(indir->gtGetOp1(), lclNum);
+                lclNode = indir->gtGetOp1()->BashToLclVar(m_compiler, lclNum);
                 break;
 
 #ifdef FEATURE_HW_INTRINSICS
@@ -1166,7 +1166,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
             {
                 GenTree*  hwiNode     = nullptr;
                 var_types elementType = indir->TypeGet();
-                lclNode               = BashToLclVar(indir->gtGetOp1(), lclNum);
+                lclNode               = indir->gtGetOp1()->BashToLclVar(m_compiler, lclNum);
 
                 if (elementType == TYP_FLOAT)
                 {
@@ -1195,7 +1195,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
 
                 GenTree*  hwiNode     = nullptr;
                 var_types elementType = indir->TypeGet();
-                lclNode               = BashToLclVar(indir, lclNum);
+                lclNode               = indir->BashToLclVar(m_compiler, lclNum);
                 GenTree* simdLclNode  = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
                 GenTree* elementNode  = user->gtGetOp2();
 
@@ -1252,7 +1252,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
                 assert(genTypeSize(varDsc) >= genTypeSize(indir));
                 assert(!isDef);
 
-                lclNode    = BashToLclVar(indir->gtGetOp1(), lclNum);
+                lclNode    = indir->gtGetOp1()->BashToLclVar(m_compiler, lclNum);
                 *val.Use() = m_compiler->gtNewCastNode(genActualType(indir), lclNode, false, indir->TypeGet());
                 break;
 
@@ -1670,27 +1670,6 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
         return (user == nullptr) || (user->OperIs(GT_COMMA) && (user->AsOp()->gtGetOp1() == node));
     }
 
-    //------------------------------------------------------------------------
-    // BashToLclVar: Bash node to a LCL_VAR.
-    //
-    // Arguments:
-    //    node   - the node to bash
-    //    lclNum - the local's number
-    //
-    // Return Value:
-    //    The bashed node.
-    //
-    GenTreeLclVar* BashToLclVar(GenTree* node, unsigned lclNum)
-    {
-        LclVarDsc* varDsc = m_compiler->lvaGetDesc(lclNum);
-
-        node->ChangeOper(GT_LCL_VAR);
-        node->ChangeType(varDsc->lvNormalizeOnLoad() ? varDsc->TypeGet() : genActualType(varDsc));
-        node->AsLclVar()->SetLclNum(lclNum);
-
-        return node->AsLclVar();
-    }
-
     void SequenceLocal(GenTreeLclVarCommon* lcl)
     {
         if (m_sequencer != nullptr)

src/coreclr/jit/runtimelookup.cpp

@@ -121,17 +121,18 @@ PhaseStatus Compiler::fgExpandRuntimeLookups()
                 block->bbFlags |= originalFlags & (BBF_SPLIT_GAINED | BBF_IMPORTED | BBF_GC_SAFE_POINT |
                                                    BBF_LOOP_PREHEADER | BBF_RETLESS_CALL);
 
+                // We've just split a block (e.g. in the middle of it) into two blocks.
+                // We have to do the same for the current statement - move all side effects before the runtime
+                // lookup to prevBb
+                gtSplitTree(block, stmt, call);
+
                 // Define a local for the result
                 const unsigned rtLookupLclNum   = lvaGrabTemp(true DEBUGARG("runtime lookup"));
                 lvaTable[rtLookupLclNum].lvType = TYP_I_IMPL;
                 GenTreeLclVar* rtLookupLcl      = gtNewLclvNode(rtLookupLclNum, call->TypeGet());
 
                 // Save expression to a local and append as the last statement in prevBb
                 auto spillExpr = [&](GenTree* expr) -> GenTree* {
-                    if (expr->OperIs(GT_LCL_VAR))
-                    {
-                        return gtClone(expr);
-                    }
                     unsigned const tmpNum   = lvaGrabTemp(false DEBUGARG("spilling expr"));
                     lvaTable[tmpNum].lvType = expr->TypeGet();
                     Statement* asgStmt      = fgNewStmtAtEnd(prevBb, gtNewTempAssign(tmpNum, expr));
@@ -313,7 +314,7 @@ PhaseStatus Compiler::fgExpandRuntimeLookups()
                 }
 
                 // Replace call with rtLookupLclNum local
-                call->ReplaceWith(gtNewLclvNode(rtLookupLclNum, call->TypeGet()), this);
+                call->BashToLclVar(this, rtLookupLclNum);
                 gtUpdateTreeAncestorsSideEffects(call);
                 gtSetStmtInfo(stmt);
                 fgSetStmtSeq(stmt);