PR feedback from SingleAccretion

Reorder invariant nodes in simple scenarios in stackifier

Jitdump when moving nodes in stackifier

When regallocwasm creates a new store node, lower it
Apply regallocwasm fix from andy

Checkpoint

Checkpoint

Add comment

Speculatively implement the dstOnStack optimization (code that hits it doesn't compile yet)

Author: kg

src/coreclr/jit/codegenwasm.cpp

@@ -13,6 +13,8 @@
 static const int LINEAR_MEMORY_INDEX = 0;
 
 #ifdef TARGET_64BIT
+static const instruction INS_I_load  = INS_i64_load;
+static const instruction INS_I_store = INS_i64_store;
 static const instruction INS_I_const = INS_i64_const;
 static const instruction INS_I_add   = INS_i64_add;
 static const instruction INS_I_mul   = INS_i64_mul;
@@ -21,6 +23,8 @@ static const instruction INS_I_le_u  = INS_i64_le_u;
 static const instruction INS_I_ge_u  = INS_i64_ge_u;
 static const instruction INS_I_gt_u  = INS_i64_gt_u;
 #else  // !TARGET_64BIT
+static const instruction INS_I_load  = INS_i32_load;
+static const instruction INS_I_store = INS_i32_store;
 static const instruction INS_I_const = INS_i32_const;
 static const instruction INS_I_add   = INS_i32_add;
 static const instruction INS_I_mul   = INS_i32_mul;
@@ -427,7 +431,9 @@ void CodeGen::WasmProduceReg(GenTree* node)
 //
 // If the operand is a candidate, we use that candidate's current register.
 // Otherwise it must have been allocated into a temporary register initialized
-// in 'WasmProduceReg'.
+// in 'WasmProduceReg'. To do this, set the LIR::Flags::MultiplyUsed flag during
+// lowering or other pre-regalloc phases, and ensure that regalloc is updated to
+// call CollectReferences on the node(s) that need to be used multiple times.
 //
 // Arguments:
 //    operand - The operand node
@@ -2420,9 +2426,98 @@ void CodeGen::genCodeForStoreBlk(GenTreeBlk* blkOp)
     }
 }
 
+//------------------------------------------------------------------------
+// genCodeForCpObj: Produce code for a GT_STORE_BLK node that represents a cpobj operation.
+//
+// Arguments:
+//    cpObjNode - the node
+//
 void CodeGen::genCodeForCpObj(GenTreeBlk* cpObjNode)
 {
-    NYI_WASM("genCodeForCpObj");
+    GenTree*  dstAddr       = cpObjNode->Addr();
+    GenTree*  source        = cpObjNode->Data();
+    var_types srcAddrType   = TYP_BYREF;
+
+    assert(source->isContained());
+    if (source->OperIs(GT_IND))
+    {
+        source = source->gtGetOp1();
+        assert(!source->isContained());
+        srcAddrType = source->TypeGet();
+    }
+
+    noway_assert(source->IsLocal());
+    noway_assert(dstAddr->IsLocal());
+
+    // If the destination is on the stack we don't need the write barrier.
+    bool dstOnStack = cpObjNode->IsAddressNotOnHeap(m_compiler);
+
+#ifdef DEBUG
+    assert(!dstAddr->isContained());
+
+    // This GenTree node has data about GC pointers, this means we're dealing
+    // with CpObj.
+    assert(cpObjNode->GetLayout()->HasGCPtr());
+#endif // DEBUG
+
+    genConsumeRegs(cpObjNode);
+
+    ClassLayout* layout = cpObjNode->GetLayout();
+    unsigned     slots  = layout->GetSlotCount();
+
+    regNumber srcReg = GetMultiUseOperandReg(source);
+    regNumber dstReg = GetMultiUseOperandReg(dstAddr);
+
+    if (cpObjNode->IsVolatile())
+    {
+        // TODO-WASM: Memory barrier
+    }
+
+    emitter* emit = GetEmitter();
+
+    emitAttr attrSrcAddr = emitActualTypeSize(srcAddrType);
+    emitAttr attrDstAddr = emitActualTypeSize(dstAddr->TypeGet());
+
+    unsigned gcPtrCount = cpObjNode->GetLayout()->GetGCPtrCount();
+
+    unsigned i = 0, offset = 0;
+    while (i < slots)
+    {
+        // Copy the pointer-sized non-gc-pointer slots one at a time (and GC pointer slots if the destination is stack)
+        //  using regular I-sized load/store pairs.
+        if (dstOnStack || !layout->IsGCPtr(i))
+        {
+            // Do a pointer-sized load+store pair at the appropriate offset relative to dest and source
+            emit->emitIns_I(INS_local_get, attrDstAddr, WasmRegToIndex(dstReg));
+            emit->emitIns_I(INS_local_get, attrSrcAddr, WasmRegToIndex(srcReg));
+            emit->emitIns_I(INS_I_load, EA_PTRSIZE, offset);
+            emit->emitIns_I(INS_I_store, EA_PTRSIZE, offset);
+        }
+        else
+        {
+            // Compute the actual dest/src of the slot being copied to pass to the helper.
+            emit->emitIns_I(INS_local_get, attrDstAddr, WasmRegToIndex(dstReg));
+            emit->emitIns_I(INS_i32_const, attrDstAddr, offset);
+            emit->emitIns(INS_i32_add);
+            emit->emitIns_I(INS_local_get, attrSrcAddr, WasmRegToIndex(srcReg));
+            emit->emitIns_I(INS_i32_const, attrSrcAddr, offset);
+            emit->emitIns(INS_i32_add);
+            // Call the byref assign helper. On other targets this updates the dst/src regs but here it won't,
+            //  so we have to do the local.get+i32.const+i32.add dance every time.
+            genEmitHelperCall(CORINFO_HELP_ASSIGN_BYREF, 0, EA_PTRSIZE);
+            gcPtrCount--;
+        }
+        ++i;
+        offset += TARGET_POINTER_SIZE;
+    }
+    assert(gcPtrCount == 0);
+
+    if (cpObjNode->IsVolatile())
+    {
+        // TODO-WASM: Memory barrier
+    }
+
+    WasmProduceReg(cpObjNode);
 }
 
 //------------------------------------------------------------------------

src/coreclr/jit/compiler.h

@@ -2566,6 +2566,10 @@ class Compiler
     friend class ReplaceVisitor;
     friend class FlowGraphNaturalLoop;
 
+#ifdef TARGET_WASM
+    friend class WasmRegAlloc; // For m_pLowering
+#endif
+
 #ifdef FEATURE_HW_INTRINSICS
     friend struct GenTreeHWIntrinsic;
     friend struct HWIntrinsicInfo;

src/coreclr/jit/gentree.cpp

@@ -12844,7 +12844,7 @@ void Compiler::gtDispTree(GenTree*                    tree,
 
 #ifdef TARGET_WASM
                     case GenTreeBlk::BlkOpKindNativeOpcode:
-                        printf(" (memory.copy|fill)");
+                        printf(" (memory.%s)", tree->OperIsCopyBlkOp() ? "copy" : "fill");
                         break;
 #endif
 

src/coreclr/jit/lowerwasm.cpp

@@ -254,6 +254,11 @@ void Lowering::LowerBlockStore(GenTreeBlk* blkNode)
             }
 
             blkNode->gtBlkOpKind = GenTreeBlk::BlkOpKindCpObjUnroll;
+            dstAddr->gtLIRFlags |= LIR::Flags::MultiplyUsed;
+            if (src->OperIs(GT_IND))
+                src->gtGetOp1()->gtLIRFlags |= LIR::Flags::MultiplyUsed;
+            else
+                src->gtLIRFlags |= LIR::Flags::MultiplyUsed;
         }
         else
         {
@@ -512,6 +517,14 @@ void Lowering::AfterLowerBlock()
                     // instead be ifdef-ed out for WASM.
                     m_anyChanges = true;
 
+                    if (node->IsInvariant())
+                    {
+                        JITDUMP("Stackifier moving invariant node [%06u] after [%06u]\n", Compiler::dspTreeID(node), Compiler::dspTreeID(prev));
+                        m_lower->BlockRange().Remove(node);
+                        m_lower->BlockRange().InsertAfter(prev, node);
+                        break;
+                    }
+
                     JITDUMP("node==[%06u] prev==[%06u]\n", Compiler::dspTreeID(node), Compiler::dspTreeID(prev));
                     NYI_WASM("IR not in a stackified form");
                 }

src/coreclr/jit/regallocwasm.cpp

@@ -8,6 +8,8 @@
 
 #include "regallocwasm.h"
 
+#include "lower.h" // for LowerRange()
+
 RegAllocInterface* GetRegisterAllocator(Compiler* compiler)
 {
     return new (compiler->getAllocator(CMK_LSRA)) WasmRegAlloc(compiler);
@@ -328,6 +330,9 @@ void WasmRegAlloc::CollectReferencesForNode(GenTree* node)
         case GT_SUB:
         case GT_MUL:
             CollectReferencesForBinop(node->AsOp());
+
+        case GT_STORE_BLK:
+            CollectReferencesForBlockStore(node->AsBlk());
             break;
 
         default:
@@ -417,6 +422,21 @@ void WasmRegAlloc::CollectReferencesForBinop(GenTreeOp* binopNode)
     ConsumeTemporaryRegForOperand(binopNode->gtGetOp1() DEBUGARG("binop overflow check"));
 }
 
+// CollectReferencesForBlockStore: Collect virtual register references for a block store.
+//
+// Arguments:
+//    node - The GT_STORE_BLK node
+//
+void WasmRegAlloc::CollectReferencesForBlockStore(GenTreeBlk* node)
+{
+    GenTree* src = node->Data();
+    if (src->OperIs(GT_IND))
+        src = src->gtGetOp1();
+
+    ConsumeTemporaryRegForOperand(src DEBUGARG("block store source"));
+    ConsumeTemporaryRegForOperand(node->Addr() DEBUGARG("block store destination"));
+}
+
 //------------------------------------------------------------------------
 // CollectReferencesForLclVar: Collect virtual register references for a LCL_VAR.
 //
@@ -476,6 +496,9 @@ void WasmRegAlloc::RewriteLocalStackStore(GenTreeLclVarCommon* lclNode)
     CurrentRange().InsertAfter(lclNode, store);
     CurrentRange().Remove(lclNode);
     CurrentRange().InsertBefore(insertionPoint, lclNode);
+
+    auto tempRange = LIR::ReadOnlyRange(store, store);
+    m_compiler->m_pLowering->LowerRange(m_currentBlock, tempRange);
 }
 
 //------------------------------------------------------------------------
@@ -539,6 +562,8 @@ void WasmRegAlloc::RequestTemporaryRegisterForMultiplyUsedNode(GenTree* node)
     // Note how due to the fact we're processing nodes in stack order,
     // we don't need to maintain free/busy sets, only a simple stack.
     regNumber reg = AllocateTemporaryRegister(genActualType(node));
+    // If the node already has a regnum, trying to assign it a second one is no good.
+    assert(node->GetRegNum() == REG_NA);
     node->SetRegNum(reg);
 }
 
@@ -561,6 +586,7 @@ void WasmRegAlloc::ConsumeTemporaryRegForOperand(GenTree* operand DEBUGARG(const
     }
 
     regNumber reg = ReleaseTemporaryRegister(genActualType(operand));
+    // If this assert fails you likely called ConsumeTemporaryRegForOperand on your operands in the wrong order.
     assert(reg == operand->GetRegNum());
     CollectReference(operand);
 
@@ -605,6 +631,7 @@ void WasmRegAlloc::ResolveReferences()
     {
         TemporaryRegStack& temporaryRegs          = m_temporaryRegs[static_cast<unsigned>(type)];
         TemporaryRegBank&  allocatedTemporaryRegs = temporaryRegMap[static_cast<unsigned>(type)];
+        // If temporaryRegs.Count != 0 that means CollectReferences failed to CollectReference one or more multiply-used nodes.
         assert(temporaryRegs.Count == 0);
 
         allocatedTemporaryRegs.Count = temporaryRegs.MaxCount;

src/coreclr/jit/regallocwasm.h

@@ -125,6 +125,7 @@ class WasmRegAlloc : public RegAllocInterface
     void      CollectReferencesForCast(GenTreeOp* castNode);
     void      CollectReferencesForBinop(GenTreeOp* binOpNode);
     void      CollectReferencesForLclVar(GenTreeLclVar* lclVar);
+    void      CollectReferencesForBlockStore(GenTreeBlk* node);
     void      RewriteLocalStackStore(GenTreeLclVarCommon* node);
     void      CollectReference(GenTree* node);
     void      RequestTemporaryRegisterForMultiplyUsedNode(GenTree* node);