More Code Cleanup for EH WriteThru (#1180)

* More Code Cleanup for EH WriteThru

These changes arose from the code reviews for the EH WriteThru work. Some are only tangentially related to EH write-thru, were suggested during that review.

* Revert change to hasEHBoundaryIn condition

* PR feedback

* More PR Feedback

* Fix comment

src/coreclr/src/jit/codegen.h

@@ -1314,12 +1314,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
 
     void inst_TT(instruction ins, GenTree* tree, unsigned offs = 0, int shfv = 0, emitAttr size = EA_UNKNOWN);
 
-    void inst_TT_RV(instruction ins,
-                    GenTree*    tree,
-                    regNumber   reg,
-                    unsigned    offs  = 0,
-                    emitAttr    size  = EA_UNKNOWN,
-                    insFlags    flags = INS_FLAGS_DONT_CARE);
+    void inst_TT_RV(instruction ins, emitAttr size, GenTree* tree, regNumber reg);
 
     void inst_RV_TT(instruction ins,
                     regNumber   reg,

src/coreclr/src/jit/codegencommon.cpp

@@ -3799,7 +3799,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
         varDsc = compiler->lvaTable + varNum;
 
 #ifndef _TARGET_64BIT_
-        // If not a stack arg go to the next one
+        // If this arg is never on the stack, go to the next one.
         if (varDsc->lvType == TYP_LONG)
         {
             if (regArgTab[argNum].slot == 1 && !regArgTab[argNum].stackArg)
@@ -3814,7 +3814,7 @@ void CodeGen::genFnPrologCalleeRegArgs(regNumber xtraReg, bool* pXtraRegClobbere
         else
 #endif // !_TARGET_64BIT_
         {
-            // If not a stack arg go to the next one
+            // If this arg is never on the stack, go to the next one.
             if (!regArgTab[argNum].stackArg)
             {
                 continue;

src/coreclr/src/jit/codegenlinear.cpp

@@ -825,7 +825,7 @@ void CodeGen::genSpillVar(GenTree* tree)
 
         instruction storeIns = ins_Store(lclTyp, compiler->isSIMDTypeLocalAligned(varNum));
         assert(varDsc->GetRegNum() == tree->GetRegNum());
-        inst_TT_RV(storeIns, tree, tree->GetRegNum(), 0, size);
+        inst_TT_RV(storeIns, size, tree, tree->GetRegNum());
 
         genUpdateRegLife(varDsc, /*isBorn*/ false, /*isDying*/ true DEBUGARG(tree));
         gcInfo.gcMarkRegSetNpt(varDsc->lvRegMask());
@@ -1853,7 +1853,8 @@ void CodeGen::genProduceReg(GenTree* tree)
             unsigned varNum = tree->AsLclVarCommon()->GetLclNum();
             assert(!compiler->lvaTable[varNum].lvNormalizeOnStore() ||
                    (tree->TypeGet() == genActualType(compiler->lvaTable[varNum].TypeGet())));
-            inst_TT_RV(ins_Store(tree->gtType, compiler->isSIMDTypeLocalAligned(varNum)), tree, tree->GetRegNum());
+            inst_TT_RV(ins_Store(tree->gtType, compiler->isSIMDTypeLocalAligned(varNum)), emitTypeSize(tree->TypeGet()),
+                       tree, tree->GetRegNum());
         }
         else
         {

src/coreclr/src/jit/codegenxarch.cpp

@@ -1366,7 +1366,7 @@ void CodeGen::genFloatReturn(GenTree* treeNode)
         {
             op1->gtFlags |= GTF_SPILL;
             inst_TT_RV(ins_Store(op1->gtType, compiler->isSIMDTypeLocalAligned(op1->AsLclVarCommon()->GetLclNum())),
-                       op1, op1->GetRegNum());
+                       emitTypeSize(op1->TypeGet()), op1, op1->GetRegNum());
         }
         // Now, load it to the fp stack.
         GetEmitter()->emitIns_S(INS_fld, emitTypeSize(op1), op1->AsLclVarCommon()->GetLclNum(), 0);

src/coreclr/src/jit/compiler.h

@@ -2995,6 +2995,7 @@ class Compiler
     // Getters and setters for address-exposed and do-not-enregister local var properties.
     bool lvaVarAddrExposed(unsigned varNum);
     void lvaSetVarAddrExposed(unsigned varNum);
+    void lvaSetVarLiveInOutOfHandler(unsigned varNum);
     bool lvaVarDoNotEnregister(unsigned varNum);
 #ifdef DEBUG
     // Reasons why we can't enregister.  Some of these correspond to debug properties of local vars.

src/coreclr/src/jit/instr.cpp

@@ -648,118 +648,41 @@ void CodeGen::inst_TT(instruction ins, GenTree* tree, unsigned offs, int shfv, e
     }
 }
 
-/*****************************************************************************
- *
- *  Generate an instruction that has one operand given by a tree (which has
- *  been made addressable) and another that is a register.
- */
-
-void CodeGen::inst_TT_RV(instruction ins, GenTree* tree, regNumber reg, unsigned offs, emitAttr size, insFlags flags)
+//------------------------------------------------------------------------
+// inst_TT_RV: Generate a store of a lclVar
+//
+// Arguments:
+//    ins  - the instruction to generate
+//    size - the size attributes for the store
+//    tree - the lclVar node
+//    reg  - the register currently holding the value of the local
+//
+void CodeGen::inst_TT_RV(instruction ins, emitAttr size, GenTree* tree, regNumber reg)
 {
+#ifdef DEBUG
+    // The tree must have a valid register value.
     assert(reg != REG_STK);
-
-AGAIN:
-
-    /* Is this a spilled value? */
-
-    if (tree->gtFlags & GTF_SPILLED)
+    bool isValidInReg = ((tree->gtFlags & GTF_SPILLED) == 0);
+    if (!isValidInReg)
     {
-        assert(!"ISSUE: If this can happen, we need to generate 'ins [ebp+spill]'");
-    }
-
-    if (size == EA_UNKNOWN)
-    {
-        if (instIsFP(ins))
+        // Is this the special case of a write-thru lclVar?
+        // We mark it as SPILLED to denote that its value is valid in memory.
+        if (((tree->gtFlags & GTF_SPILL) != 0) && tree->gtOper == GT_STORE_LCL_VAR)
         {
-            size = EA_ATTR(genTypeSize(tree->TypeGet()));
-        }
-        else
-        {
-            size = emitTypeSize(tree->TypeGet());
+            isValidInReg = true;
         }
     }
+    assert(isValidInReg);
+    assert(size != EA_UNKNOWN);
+    assert(tree->OperIs(GT_LCL_VAR, GT_STORE_LCL_VAR));
+#endif // DEBUG
 
-    switch (tree->gtOper)
-    {
-        unsigned varNum;
-
-        case GT_LCL_VAR:
-
-            inst_set_SV_var(tree);
-            goto LCL;
-
-        case GT_LCL_FLD:
-        case GT_STORE_LCL_FLD:
-            offs += tree->AsLclFld()->GetLclOffs();
-            goto LCL;
-
-        LCL:
-
-            varNum = tree->AsLclVarCommon()->GetLclNum();
-            assert(varNum < compiler->lvaCount);
-
+    unsigned varNum = tree->AsLclVarCommon()->GetLclNum();
+    assert(varNum < compiler->lvaCount);
 #if CPU_LOAD_STORE_ARCH
-            if (!GetEmitter()->emitInsIsStore(ins))
-            {
-                // TODO-LdStArch-Bug: Should regTmp be a dst on the node or an internal reg?
-                // Either way, it is not currently being handled by Lowering.
-                regNumber regTmp = tree->GetRegNum();
-                assert(regTmp != REG_NA);
-                GetEmitter()->emitIns_R_S(ins_Load(tree->TypeGet()), size, regTmp, varNum, offs);
-                GetEmitter()->emitIns_R_R(ins, size, regTmp, reg, flags);
-                GetEmitter()->emitIns_S_R(ins_Store(tree->TypeGet()), size, regTmp, varNum, offs);
-
-                regSet.verifyRegUsed(regTmp);
-            }
-            else
+    assert(GetEmitter()->emitInsIsStore(ins));
 #endif
-            {
-                // ins is a Store instruction
-                //
-                GetEmitter()->emitIns_S_R(ins, size, reg, varNum, offs);
-#ifdef _TARGET_ARM_
-                // If we need to set the flags then add an extra movs reg,reg instruction
-                if (flags == INS_FLAGS_SET)
-                    GetEmitter()->emitIns_R_R(INS_mov, size, reg, reg, INS_FLAGS_SET);
-#endif
-            }
-            return;
-
-        case GT_CLS_VAR:
-            // Make sure FP instruction size matches the operand size
-            // (We optimized constant doubles to floats when we can, just want to
-            // make sure that we don't mistakenly use 8 bytes when the
-            // constant).
-            assert(!isFloatRegType(tree->gtType) || genTypeSize(tree->gtType) == EA_SIZE_IN_BYTES(size));
-
-#if CPU_LOAD_STORE_ARCH
-            if (!GetEmitter()->emitInsIsStore(ins))
-            {
-                NYI("Store of GT_CLS_VAR not supported for ARM");
-            }
-            else
-#endif // CPU_LOAD_STORE_ARCH
-            {
-                GetEmitter()->emitIns_C_R(ins, size, tree->AsClsVar()->gtClsVarHnd, reg, offs);
-            }
-            return;
-
-        case GT_IND:
-        case GT_NULLCHECK:
-        case GT_ARR_ELEM:
-        {
-            assert(!"inst_TT_RV not supported for GT_IND, GT_NULLCHECK or GT_ARR_ELEM");
-        }
-        break;
-
-        case GT_COMMA:
-            //     tree->AsOp()->gtOp1 - already processed by genCreateAddrMode()
-            tree = tree->AsOp()->gtOp2;
-            goto AGAIN;
-
-        default:
-            assert(!"invalid address");
-    }
+    GetEmitter()->emitIns_S_R(ins, size, reg, varNum, 0);
 }
 
 /*****************************************************************************

src/coreclr/src/jit/lclvars.cpp

@@ -2369,6 +2369,33 @@ void Compiler::lvaSetVarAddrExposed(unsigned varNum)
     lvaSetVarDoNotEnregister(varNum DEBUGARG(DNER_AddrExposed));
 }
 
+//------------------------------------------------------------------------
+// lvaSetVarLiveInOutOfHandler: Set the local varNum as being live in and/or out of a handler
+//
+// Arguments:
+//    varNum - the varNum of the local
+//
+void Compiler::lvaSetVarLiveInOutOfHandler(unsigned varNum)
+{
+    LclVarDsc* varDsc = lvaGetDesc(varNum);
+
+    INDEBUG(varDsc->lvLiveInOutOfHndlr = 1);
+
+    if (varDsc->lvPromoted)
+    {
+        noway_assert(varTypeIsStruct(varDsc));
+
+        for (unsigned i = varDsc->lvFieldLclStart; i < varDsc->lvFieldLclStart + varDsc->lvFieldCnt; ++i)
+        {
+            noway_assert(lvaTable[i].lvIsStructField);
+            INDEBUG(lvaTable[i].lvLiveInOutOfHndlr = 1);
+            lvaSetVarDoNotEnregister(i DEBUGARG(DNER_LiveInOutOfHandler));
+        }
+    }
+
+    lvaSetVarDoNotEnregister(varNum DEBUGARG(DNER_LiveInOutOfHandler));
+}
+
 /*****************************************************************************
  *
  *  Record that the local var "varNum" should not be enregistered (for one of several reasons.)
@@ -6779,6 +6806,11 @@ void Compiler::lvaDumpEntry(unsigned lclNum, FrameLayoutState curState, size_t r
         printf(" HFA(%s) ", varTypeName(varDsc->GetHfaType()));
     }
 
+    if (varDsc->lvLiveInOutOfHndlr)
+    {
+        printf(" EH");
+    }
+
     if (varDsc->lvDoNotEnregister)
     {
         printf(" do-not-enreg[");

src/coreclr/src/jit/liveness.cpp

@@ -2472,37 +2472,26 @@ void Compiler::fgInterBlockLocalVarLiveness()
         }
 
         // Mark all variables that are live on entry to an exception handler
-        // or on exit from a filter handler or finally as DoNotEnregister */
+        // or on exit from a filter handler or finally.
 
-        if (VarSetOps::IsMember(this, exceptVars, varDsc->lvVarIndex) ||
+        bool isFinallyVar = VarSetOps::IsMember(this, finallyVars, varDsc->lvVarIndex);
+        if (isFinallyVar || VarSetOps::IsMember(this, exceptVars, varDsc->lvVarIndex) ||
             VarSetOps::IsMember(this, filterVars, varDsc->lvVarIndex))
         {
-            /* Mark the variable appropriately */
-            lvaSetVarDoNotEnregister(varNum DEBUGARG(DNER_LiveInOutOfHandler));
-        }
-
-        /* Mark all pointer variables live on exit from a 'finally'
-           block as either volatile for non-GC ref types or as
-           'explicitly initialized' (volatile and must-init) for GC-ref types */
-
-        if (VarSetOps::IsMember(this, finallyVars, varDsc->lvVarIndex))
-        {
-            lvaSetVarDoNotEnregister(varNum DEBUGARG(DNER_LiveInOutOfHandler));
+            // Mark the variable appropriately.
+            lvaSetVarLiveInOutOfHandler(varNum);
 
-            /* Don't set lvMustInit unless we have a non-arg, GC pointer */
+            // Mark all pointer variables live on exit from a 'finally' block as
+            // 'explicitly initialized' (must-init) for GC-ref types.
 
-            if (varDsc->lvIsParam)
+            if (isFinallyVar)
             {
-                continue;
-            }
-
-            if (!varTypeIsGC(varDsc->TypeGet()))
-            {
-                continue;
+                // Set lvMustInit only if we have a non-arg, GC pointer.
+                if (!varDsc->lvIsParam && varTypeIsGC(varDsc->TypeGet()))
+                {
+                    varDsc->lvMustInit = true;
+                }
             }
-
-            /* Mark it */
-            varDsc->lvMustInit = true;
         }
     }