Less Conservative GtObj

Don't mark GT_OBJ with GTF_EXCEPT or GTF_GLOB_REF if it is a local struct.
Also, fix changes needed in arm register allocation and code generation to handle the case where a struct argument is promoted, which appears to be code that was never exercised.
These changes are in preparation for further changes for First Class Structs. I am working to replace block ops with assignments, and was attempting to get to zero diffs, but it is quite difficult to do so given some inconsistencies in the treatment of GT_OBJs of locals.

Commit migrated from https://github.com/dotnet/coreclr/commit/74b27523b5a150e4cac25d43b37efbbcad008020

diff --git a/src/coreclr/src/jit/codegenlegacy.cpp b/src/coreclr/src/jit/codegenlegacy.cpp
index 9f242b6..476f8af 100644 (file)
--- a/src/coreclr/src/jit/codegenlegacy.cpp
+++ b/src/coreclr/src/jit/codegenlegacy.cpp
@@ -18328,7 +18328,8 @@ void CodeGen::SetupLateArgs(GenTreePtr call)
             {
                 // First pass the stack portion of the struct (if any)
                 //
-                for (unsigned i = firstStackSlot; i < slots; i++)
+                               int argOffsetOfFirstStackSlot = argOffset;
+                               for (unsigned i = firstStackSlot; i < slots; i++)
                 {
                     emitAttr fieldSize;
                     if      (gcLayout[i] == TYPE_GC_NONE)
@@ -18356,7 +18357,7 @@ void CodeGen::SetupLateArgs(GenTreePtr call)
                                                           /*pCurRegNum*/&maxRegArg,
                                                           argOffset, 
                                                           /*fieldOffsetOfFirstStackSlot*/ firstStackSlot * TARGET_POINTER_SIZE,
-                                                          /*argOffsetOfFirstStackSlot*/ 0, // is always zero in this "spanning" case.
+                                                          argOffsetOfFirstStackSlot,
                                                           &deadFieldVarRegs,
                                                           &regTmp);
                         if (filledExtraSlot) 

diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h
index b0125d9..cf8340a 100644 (file)
--- a/src/coreclr/src/jit/compiler.h
+++ b/src/coreclr/src/jit/compiler.h
@@ -6822,7 +6822,7 @@ public :
 #endif // _TARGET_ARM_
 
     // If "tree" is a indirection (GT_IND, or GT_OBJ) whose arg is an ADDR, whose arg is a LCL_VAR, return that LCL_VAR node, else NULL.
-    GenTreePtr          fgIsIndirOfAddrOfLocal(GenTreePtr tree);
+    static GenTreePtr   fgIsIndirOfAddrOfLocal(GenTreePtr tree);
 
     // This is indexed by GT_OBJ nodes that are address of promoted struct variables, which
     // have been annotated with the GTF_VAR_DEATH flag.  If such a node is *not* mapped in this

diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp
index baf3575..0efa42e 100644 (file)
--- a/src/coreclr/src/jit/flowgraph.cpp
+++ b/src/coreclr/src/jit/flowgraph.cpp
@@ -20641,7 +20641,7 @@ void                Compiler::fgDebugCheckFlags(GenTreePtr tree)
     if (chkFlags & ~treeFlags)
     {
         // Print the tree so we can see it in the log.
-        printf("Missing flags on tree [%X]: ", tree);
+        printf("Missing flags on tree [%06d]: ", dspTreeID(tree));
         GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
@@ -20649,7 +20649,7 @@ void                Compiler::fgDebugCheckFlags(GenTreePtr tree)
         noway_assert(!"Missing flags on tree");
 
         // Print the tree again so we can see it right after we hook up the debugger.
-        printf("Missing flags on tree [%X]: ", tree);
+        printf("Missing flags on tree [%06d]: ", dspTreeID(tree));
         GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
@@ -22403,9 +22403,11 @@ GenTreePtr      Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
                     !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) &&
                     !inlArgInfo[argNum].argHasLdargaOp)
                 {
-                    /* Change the temp in-place to the actual argument */
-
-                    argSingleUseNode->CopyFrom(inlArgInfo[argNum].argNode, this);
+                    // Change the temp in-place to the actual argument.
+                    // We currently do not support this for struct arguments, so it must not be a GT_OBJ.
+                    GenTree* argNode = inlArgInfo[argNum].argNode;
+                    assert(argNode->gtOper != GT_OBJ);
+                    argSingleUseNode->CopyFrom(argNode, this);
                     continue;
                 }
                 else

diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp
index 947fde6..9446630 100644 (file)
--- a/src/coreclr/src/jit/gentree.cpp
+++ b/src/coreclr/src/jit/gentree.cpp
@@ -5333,12 +5333,14 @@ bool                GenTree::OperMayThrow()
 
         break;
 
+    case GT_OBJ:
+        return !Compiler::fgIsIndirOfAddrOfLocal(this);
+
     case GT_ARR_BOUNDS_CHECK:
     case GT_ARR_ELEM:
     case GT_ARR_INDEX:
     case GT_CATCH_ARG:
     case GT_ARR_LENGTH:
-    case GT_OBJ:
     case GT_LCLHEAP:
     case GT_CKFINITE:
     case GT_NULLCHECK:
@@ -6052,7 +6054,14 @@ GenTreeObj* Compiler::gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr
 {
     var_types nodeType   = impNormStructType(structHnd);
     assert(varTypeIsStruct(nodeType));
-    return new (this, GT_OBJ) GenTreeObj(nodeType, addr, structHnd);
+    GenTreeObj *objNode = new (this, GT_OBJ) GenTreeObj(nodeType, addr, structHnd);
+    // An Obj is not a global reference, if it is known to be a local struct.
+    GenTreeLclVarCommon* lclNode = addr->IsLocalAddrExpr();
+    if ((lclNode != nullptr) && !lvaIsImplicitByRefLocal(lclNode->gtLclNum))
+    {
+        objNode->gtFlags &= ~GTF_GLOB_REF;
+    }
+    return objNode;
 }
 
 // Creates a new CpObj node.
@@ -14795,7 +14804,9 @@ bool FieldSeqNode::IsPseudoField()
 #ifdef FEATURE_SIMD
 GenTreeSIMD* Compiler::gtNewSIMDNode(var_types type, GenTreePtr op1, SIMDIntrinsicID simdIntrinsicID, var_types baseType, unsigned size)
 {   
-    assert(op1 != nullptr);
+       // TODO-CQ: An operand may be a GT_OBJ(GT_ADDR(GT_LCL_VAR))), in which case it should be
+       // marked lvUsedInSIMDIntrinsic.
+       assert(op1 != nullptr);
     if (op1->OperGet() == GT_LCL_VAR)
     {
         unsigned lclNum = op1->AsLclVarCommon()->GetLclNum();
@@ -14808,6 +14819,8 @@ GenTreeSIMD* Compiler::gtNewSIMDNode(var_types type, GenTreePtr op1, SIMDIntrins
 
 GenTreeSIMD* Compiler::gtNewSIMDNode(var_types type, GenTreePtr op1, GenTreePtr op2, SIMDIntrinsicID simdIntrinsicID, var_types baseType, unsigned size)
 {
+    // TODO-CQ: An operand may be a GT_OBJ(GT_ADDR(GT_LCL_VAR))), in which case it should be
+    // marked lvUsedInSIMDIntrinsic.
     assert(op1 != nullptr);
     if (op1->OperIsLocal())
     {

diff --git a/src/coreclr/src/jit/importer.cpp b/src/coreclr/src/jit/importer.cpp
index d2a92ee..f9e48f7 100644 (file)
--- a/src/coreclr/src/jit/importer.cpp
+++ b/src/coreclr/src/jit/importer.cpp
@@ -1511,7 +1511,7 @@ GenTreePtr      Compiler::impNormStructVal(GenTreePtr    structVal,
 
     // Normalize it by wraping it in an OBJ
 
-    GenTreePtr structAddr  = impGetStructAddr(structVal, structHnd, curLevel, !forceNormalization); // get the addr of struct
+    GenTreePtr structAddr = impGetStructAddr(structVal, structHnd, curLevel, !forceNormalization); // get the addr of struct
     GenTreePtr structObj = new (this, GT_OBJ) GenTreeObj(structType, structAddr, structHnd);
 
     if (structAddr->gtOper == GT_ADDR)
@@ -1525,9 +1525,10 @@ GenTreePtr      Compiler::impNormStructVal(GenTreePtr    structVal,
     {
         // A OBJ on a ADDR(LCL_VAR) can never raise an exception
         // so we don't set GTF_EXCEPT here.
-        //
-        // TODO-CQ: Clear the GTF_GLOB_REF flag on structObj as well
-        //          but this needs additional work when inlining.
+        if (!lvaIsImplicitByRefLocal(structVal->AsLclVarCommon()->gtLclNum))
+        {
+            structObj->gtFlags &= ~GTF_GLOB_REF;
+        }
     }
     else  
     {
@@ -16853,15 +16854,17 @@ GenTreePtr  Compiler::impInlineFetchArg(unsigned lclNum, InlArgInfo *inlArgInfo,
             inlArgInfo[lclNum].argHasTmp = true;
             inlArgInfo[lclNum].argTmpNum = tmpNum;
             
-            /* If we require strict exception order then, arguments must
-            be evaulated in sequence before the body of the inlined
-            method. So we need to evaluate them to a temp.
-            Also, if arguments have global references, we need to
-            evaluate them to a temp before the inlined body as the
-            inlined body may be modifying the global ref.
-            */
+            // If we require strict exception order, then arguments must
+            // be evaluated in sequence before the body of the inlined method.
+            // So we need to evaluate them to a temp.
+            // Also, if arguments have global references, we need to
+            // evaluate them to a temp before the inlined body as the
+            // inlined body may be modifying the global ref.
+            // TODO-1stClassStructs: We currently do not reuse an existing lclVar
+            // if it is a struct, because it requires some additional handling.
             
-            if ((!inlArgInfo[lclNum].argHasSideEff) &&
+            if (!varTypeIsStruct(lclTyp) &&
+                (!inlArgInfo[lclNum].argHasSideEff) &&
                 (!inlArgInfo[lclNum].argHasGlobRef))
             {
                 /* Get a *LARGE* LCL_VAR node */

diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp
index 5572d8c..902fce3 100755 (executable)
--- a/src/coreclr/src/jit/morph.cpp
+++ b/src/coreclr/src/jit/morph.cpp
@@ -2085,9 +2085,9 @@ GenTreePtr    Compiler::fgMakeTmpArgNode(unsigned tmpVarNum
         arg = gtNewOperNode(GT_ADDR, TYP_BYREF, arg);
         addrNode = arg;
 
-        // Get a new Obj node temp to use it as a call argument
+        // Get a new Obj node temp to use it as a call argument.
+        // gtNewObjNode will set the GTF_EXCEPT flag if this is not a local stack object.
         arg = gtNewObjNode(lvaGetStruct(tmpVarNum), arg);
-        arg->gtFlags |= GTF_EXCEPT;
 
 #endif  // not (_TARGET_AMD64_ or _TARGET_ARM64_)
 
@@ -4409,8 +4409,6 @@ void Compiler::fgMorphSystemVStructArgs(GenTreeCall* call, bool hasStructArgumen
 
                         // Create an Obj of the temp to use it as a call argument.
                         arg = new (this, GT_OBJ) GenTreeObj(originalType, arg, lvaGetStruct(lclCommon->gtLclNum));
-                        arg->gtFlags |= GTF_EXCEPT;
-                        flagsSummary |= GTF_EXCEPT;
                     }
                 }
             }
@@ -8386,17 +8384,19 @@ ONE_SIMPLE_ASG:
             }
         }
 
-        /* Indirect the dest node */
+        // Indirect the dest node.
 
         dest = gtNewOperNode(GT_IND, type, dest);
 
-        /* As long as we don't have more information about the destination we
-           have to assume it could live anywhere (not just in the GC heap). Mark
-           the GT_IND node so that we use the correct write barrier helper in case
-           the field is a GC ref.
-        */
+        // If we have no information about the destination, we have to assume it could
+        // live anywhere (not just in the GC heap).
+        // Mark the GT_IND node so that we use the correct write barrier helper in case
+        // the field is a GC ref.
 
-        dest->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
+        if (!fgIsIndirOfAddrOfLocal(dest))
+        {
+            dest->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
+        }
 
 _DoneDest:;
 
@@ -8444,10 +8444,19 @@ _DoneDest:;
                 }
             }
 
-            /* Indirect the src node */
+            // Indirect the src node.
 
             src  = gtNewOperNode(GT_IND, type, src);
-            src->gtFlags     |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
+
+            // If we have no information about the src, we have to assume it could
+            // live anywhere (not just in the GC heap).
+            // Mark the GT_IND node so that we use the correct write barrier helper in case
+            // the field is a GC ref.
+
+            if (!fgIsIndirOfAddrOfLocal(src))
+            {
+                src->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
+            }
 
 _DoneSrc:;
 
@@ -11763,6 +11772,16 @@ CM_ADD_OP:
         fgAddCodeRef(compCurBB, bbThrowIndex(compCurBB), SCK_ARITH_EXCPN, fgPtrArgCntCur);
         break;
 
+    case GT_OBJ:
+        // If we have GT_OBJ(GT_ADDR(X)) and X has GTF_GLOB_REF, we must set GTF_GLOB_REF on
+        // the GT_OBJ. Note that the GTF_GLOB_REF will have been cleared on ADDR(X) where X
+        // is a local or clsVar, even if it has been address-exposed.
+        if (op1->OperGet() == GT_ADDR)
+        {
+            tree->gtFlags |= (op1->gtGetOp1()->gtFlags & GTF_GLOB_REF);
+        }
+        break;
+
     case GT_IND:
 
         // Can not remove a GT_IND if it is currently a CSE candidate.

diff --git a/src/coreclr/src/jit/regalloc.cpp b/src/coreclr/src/jit/regalloc.cpp
index 482a590..ec040d7 100644 (file)
--- a/src/coreclr/src/jit/regalloc.cpp
+++ b/src/coreclr/src/jit/regalloc.cpp
@@ -4693,7 +4693,7 @@ HANDLE_SHIFT_COUNT:
                     }
                 }
 
-                if (promotedStructLocal != NULL)
+                if ((promotedStructLocal != NULL) && (curArgMask != RBM_NONE))
                 {
                     // All or a portion of this struct will be placed in the argument registers indicated by
                     // "curArgMask". We build in knowledge of the order in which the code is generated here, so