JIT: Generalize handling of commas in block morphing (#83590)

Eliminate commas early in block morphing, before the rest of the
transformation needs to look at it. Do this by extracting their side
effects and adding them on top of the returned result instead. This
allows us to handle arbitrary COMMAs on destination operand in a general
manner.

Prerequisite to #83388.

Fix #1699.

Author: jakobbotsch

src/coreclr/jit/fginline.cpp

@@ -393,15 +393,8 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi
             asg                      = inlinee;
         }
 
-        // Block morphing does not support (promoted) locals under commas, as such, instead of "COMMA(asg, lcl)" we
-        // do "OBJ(COMMA(asg, ADDR(LCL)))". TODO-1stClassStructs: improve block morphing and delete this workaround.
-        //
-        GenTree* lcl  = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
-        GenTree* addr = m_compiler->gtNewOperNode(GT_ADDR, TYP_I_IMPL, lcl);
-        addr          = m_compiler->gtNewOperNode(GT_COMMA, addr->TypeGet(), asg, addr);
-        GenTree* obj  = m_compiler->gtNewObjNode(varDsc->GetLayout(), addr);
-
-        return obj;
+        GenTree* lcl = m_compiler->gtNewLclvNode(lclNum, varDsc->TypeGet());
+        return m_compiler->gtNewOperNode(GT_COMMA, lcl->TypeGet(), asg, lcl);
     }
 #endif // FEATURE_MULTIREG_RET
 

src/coreclr/jit/morphblock.cpp

@@ -27,12 +27,10 @@ class MorphInitBlockHelper
         return "MorphInitBlock";
     }
 
-    static GenTree* MorphBlock(Compiler* comp, GenTree* tree, bool isDest);
-    static GenTree* MorphCommaBlock(Compiler* comp, GenTreeOp* firstComma);
-
 private:
-    void TryInitFieldByField();
-    void TryPrimitiveInit();
+    void     TryInitFieldByField();
+    void     TryPrimitiveInit();
+    GenTree* EliminateCommas(GenTree** commaPool);
 
 protected:
     Compiler* m_comp;
@@ -127,6 +125,9 @@ GenTree* MorphInitBlockHelper::Morph()
 {
     JITDUMP("%s:\n", GetHelperName());
 
+    GenTree* commaPool;
+    GenTree* sideEffects = EliminateCommas(&commaPool);
+
     PrepareDst();
     PrepareSrc();
     PropagateBlockAssertions();
@@ -147,12 +148,33 @@ GenTree* MorphInitBlockHelper::Morph()
     {
         m_result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED;
     }
-    if (m_comp->verbose)
+#endif
+
+    while (sideEffects != nullptr)
     {
-        printf("%s (after):\n", GetHelperName());
-        m_comp->gtDispTree(m_result);
+        if (commaPool != nullptr)
+        {
+            GenTree* comma = commaPool;
+            commaPool      = commaPool->gtNext;
+
+            assert(comma->OperIs(GT_COMMA));
+            comma->AsOp()->gtOp1 = sideEffects;
+            comma->AsOp()->gtOp2 = m_result;
+            comma->gtFlags       = (sideEffects->gtFlags | m_result->gtFlags) & GTF_ALL_EFFECT;
+
+            m_result = comma;
+        }
+        else
+        {
+            m_result = m_comp->gtNewOperNode(GT_COMMA, m_result->TypeGet(), sideEffects, m_result);
+        }
+        INDEBUG(m_result->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
+
+        sideEffects = sideEffects->gtNext;
     }
-#endif // DEBUG
+
+    JITDUMP("%s (after):\n", GetHelperName());
+    DISPTREE(m_result);
 
     return m_result;
 }
@@ -317,125 +339,6 @@ void MorphInitBlockHelper::MorphStructCases()
     }
 }
 
-//------------------------------------------------------------------------
-// MorphBlock: Morph a block node preparatory to morphing a block assignment.
-//
-// Arguments:
-//    comp - a compiler instance;
-//    tree - a struct type node;
-//    isDest - true if this is the destination of an assignment;
-//
-// Return Value:
-//    Returns the possibly-morphed node. The caller is responsible for updating
-//    the parent of this node.
-//
-// static
-GenTree* MorphInitBlockHelper::MorphBlock(Compiler* comp, GenTree* tree, bool isDest)
-{
-    JITDUMP("MorphBlock for %s tree, before:\n", (isDest ? "dst" : "src"));
-    DISPTREE(tree);
-
-    assert(varTypeIsStruct(tree));
-
-    if (tree->OperIs(GT_COMMA))
-    {
-        // TODO-Cleanup: this block is not needed for not struct nodes, but
-        // TryPrimitiveCopy works wrong without this transformation.
-        tree = MorphCommaBlock(comp, tree->AsOp());
-        if (isDest)
-        {
-            tree->SetDoNotCSE();
-        }
-    }
-
-    assert(!tree->OperIsIndir() || varTypeIsI(genActualType(tree->AsIndir()->Addr())));
-
-    JITDUMP("MorphBlock after:\n");
-    DISPTREE(tree);
-    return tree;
-}
-
-//------------------------------------------------------------------------
-// MorphCommaBlock: transform COMMA<struct>(X) as OBJ<STRUCT>(COMMA byref(ADDR(X)).
-//
-// Notes:
-//    In order to CSE and value number array index expressions and bounds checks,
-//    the commas in which they are contained need to match.
-//    The pattern is that the COMMA should be the address expression.
-//    Therefore, we insert a GT_ADDR just above the node, and wrap it in an obj or ind.
-//    TODO-1stClassStructs: Consider whether this can be improved.
-//    Example:
-//      before: [3] comma struct <- [2] comma struct <- [1] LCL_VAR struct
-//      after: [5] obj <- [3] comma byref <- [2] comma byref <- [4] addr byref <- [1] LCL_VAR struct
-//
-// static
-GenTree* MorphInitBlockHelper::MorphCommaBlock(Compiler* comp, GenTreeOp* firstComma)
-{
-    assert(firstComma->OperIs(GT_COMMA));
-
-    ArrayStack<GenTree*> commas(comp->getAllocator(CMK_ArrayStack));
-    for (GenTree* currComma = firstComma; currComma != nullptr && currComma->OperIs(GT_COMMA);
-         currComma          = currComma->gtGetOp2())
-    {
-        commas.Push(currComma);
-    }
-
-    GenTree* lastComma = commas.Top();
-
-    GenTree* effectiveVal = lastComma->gtGetOp2();
-
-    if (!effectiveVal->OperIsIndir() && !effectiveVal->IsLocal())
-    {
-        return firstComma;
-    }
-
-    assert(effectiveVal == firstComma->gtEffectiveVal());
-
-    GenTree* effectiveValAddr = comp->gtNewOperNode(GT_ADDR, TYP_BYREF, effectiveVal);
-
-    INDEBUG(effectiveValAddr->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
-
-    lastComma->AsOp()->gtOp2 = effectiveValAddr;
-
-    while (!commas.Empty())
-    {
-        GenTree* comma = commas.Pop();
-        comma->gtType  = TYP_BYREF;
-
-        // The "IND(COMMA)" => "COMMA(IND)" transform may have set NO_CSEs on these COMMAs, clear them.
-        comma->ClearDoNotCSE();
-        comp->gtUpdateNodeSideEffects(comma);
-    }
-
-    const var_types blockType = effectiveVal->TypeGet();
-    GenTree*        addr      = firstComma;
-
-    GenTree* res;
-
-    if (blockType == TYP_STRUCT)
-    {
-        CORINFO_CLASS_HANDLE structHnd = comp->gtGetStructHandleIfPresent(effectiveVal);
-        if (structHnd == NO_CLASS_HANDLE)
-        {
-            // TODO-1stClassStructs: get rid of all such cases.
-            res = comp->gtNewIndir(blockType, addr);
-        }
-        else
-        {
-            res = comp->gtNewObjNode(structHnd, addr);
-            comp->gtSetObjGcInfo(res->AsObj());
-        }
-    }
-    else
-    {
-        res = comp->gtNewIndir(blockType, addr);
-    }
-
-    comp->gtUpdateNodeSideEffects(res);
-    INDEBUG(res->gtDebugFlags |= GTF_DEBUG_NODE_MORPHED);
-    return res;
-}
-
 //------------------------------------------------------------------------
 // InitFieldByField: Attempts to promote a local block init tree to a tree
 // of promoted field initialization assignments.
@@ -663,6 +566,119 @@ void MorphInitBlockHelper::TryPrimitiveInit()
     }
 }
 
+//------------------------------------------------------------------------
+// EliminateCommas: Prepare for block morphing by removing commas from the
+// source operand of the assignment.
+//
+// Parameters:
+//   commaPool - [out] Pool of GT_COMMA nodes linked by their gtNext nodes that
+//                     can be used by the caller to avoid unnecessarily creating
+//                     new commas.
+//
+// Returns:
+//   Extracted side effects, in reverse order, linked via the gtNext fields of
+//   the nodes.
+//
+// Notes:
+//   We have a tree like the following (note that location-valued commas are
+//   illegal, so there cannot be a comma on the left):
+//
+//            ASG
+//          /     \.
+//        IND   COMMA
+//         |      /  \.
+//         B     C    D
+//
+//   We'd like downstream code to just see and be expand ASG(IND(B), D).
+//   We will produce:
+//
+//       COMMA
+//       /   \.
+//     ASG   COMMA
+//    /  \    /  \.
+//   tmp  B  C   ASG
+//              /  \.
+//            IND   D
+//             |
+//            tmp
+//
+//   If the ASG has GTF_REVERSE_OPS then we will produce:
+//
+//      COMMA
+//      /   \.
+//     C    ASG
+//         /  \.
+//       IND   D
+//        |
+//        B
+//
+//   While keeping the GTF_REVERSE_OPS.
+//
+//   Note that the final resulting tree is created in the caller since it also
+//   needs to propagate side effect flags from the decomposed assignment to all
+//   the created commas. Therefore this function just returns a linked list of
+//   the side effects to be used for that purpose.
+//
+GenTree* MorphInitBlockHelper::EliminateCommas(GenTree** commaPool)
+{
+    *commaPool = nullptr;
+
+    GenTree* sideEffects = nullptr;
+    auto addSideEffect   = [&sideEffects](GenTree* sideEff) {
+        sideEff->gtNext = sideEffects;
+        sideEffects     = sideEff;
+    };
+
+    auto addComma = [commaPool, &addSideEffect](GenTree* comma) {
+        addSideEffect(comma->gtGetOp1());
+        comma->gtNext = *commaPool;
+        *commaPool    = comma;
+    };
+
+    GenTree* lhs = m_asg->gtGetOp1();
+    assert(lhs->OperIsIndir() || lhs->OperIsLocal());
+
+    GenTree* rhs = m_asg->gtGetOp2();
+
+    if (m_asg->IsReverseOp())
+    {
+        while (rhs->OperIs(GT_COMMA))
+        {
+            addComma(rhs);
+            rhs = rhs->gtGetOp2();
+        }
+    }
+    else
+    {
+        if (lhs->OperIsIndir() && rhs->OperIs(GT_COMMA))
+        {
+            GenTree* addr = lhs->gtGetOp1();
+            if (((addr->gtFlags & GTF_ALL_EFFECT) != 0) || (((rhs->gtFlags & GTF_ASG) != 0) && !addr->IsInvariant()))
+            {
+                unsigned lhsAddrLclNum = m_comp->lvaGrabTemp(true DEBUGARG("Block morph LHS addr"));
+
+                addSideEffect(m_comp->gtNewTempAssign(lhsAddrLclNum, addr));
+                lhs->AsUnOp()->gtOp1 = m_comp->gtNewLclvNode(lhsAddrLclNum, genActualType(addr));
+                m_comp->gtUpdateNodeSideEffects(lhs);
+            }
+        }
+
+        while (rhs->OperIs(GT_COMMA))
+        {
+            addComma(rhs);
+            rhs = rhs->gtGetOp2();
+        }
+    }
+
+    if (sideEffects != nullptr)
+    {
+        m_asg->gtOp2 = rhs;
+        m_comp->gtUpdateNodeSideEffects(m_asg);
+    }
+
+    return sideEffects;
+}
+
 class MorphCopyBlockHelper : public MorphInitBlockHelper
 {
 public:
@@ -733,12 +749,7 @@ MorphCopyBlockHelper::MorphCopyBlockHelper(Compiler* comp, GenTree* asg) : Morph
 //
 void MorphCopyBlockHelper::PrepareSrc()
 {
-    GenTree* origSrc = m_asg->gtGetOp2();
-    m_src            = MorphBlock(m_comp, origSrc, false);
-    if (m_src != origSrc)
-    {
-        m_asg->gtOp2 = m_src;
-    }
+    m_src = m_asg->gtGetOp2();
 
     if (m_src->IsLocal())
     {

src/coreclr/jit/rationalize.cpp

@@ -69,8 +69,8 @@ void Rationalizer::RewriteIndir(LIR::Use& use)
 // Arguments:
 //    use - A use of a GT_IND node of SIMD type
 //
-// TODO-ADDR: delete this once block morphing stops taking addresses of locals
-// under COMMAs.
+// TODO-ADDR: today this only exists because the xarch backend does not handle
+// IND<simd12>(LCL_VAR_ADDR/LCL_FLD_ADDR) when the address is contained correctly.
 //
 void Rationalizer::RewriteSIMDIndir(LIR::Use& use)
 {