Merge pull request #6227 from parjong/revert-6021-LessConservativeGtObj

Revert "Less conservative gt obj"

8 files changed, 44 insertions, 89 deletions

diff --git a/src/jit/codegenlegacy.cpp b/src/jit/codegenlegacy.cpp
index 6db43addaf..0b93c6d243 100644
--- a/src/jit/codegenlegacy.cpp
+++ b/src/jit/codegenlegacy.cpp
@@ -17725,7 +17725,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                             fieldSize,
                                             fieldVarDsc->lvRegNum,
                                             compiler->lvaOutgoingArgSpaceVar,
-                                            argOffset + fieldArgOffset);
+                                            fieldArgOffset);
 
                     if (fieldVarDsc->lvExactSize == 8) 
                     {
@@ -17751,7 +17751,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                                         fieldSize,
                                                         regTmp,
                                                         compiler->lvaOutgoingArgSpaceVar,
-                                                        argOffset + fieldArgOffset + TARGET_POINTER_SIZE);
+                                                        fieldArgOffset + TARGET_POINTER_SIZE);
                             }
                             else
                             {
@@ -17759,7 +17759,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                                         fieldSize,
                                                         fieldVarDsc->lvOtherReg,
                                                         compiler->lvaOutgoingArgSpaceVar,
-                                                        argOffset + fieldArgOffset + TARGET_POINTER_SIZE);
+                                                        fieldArgOffset + TARGET_POINTER_SIZE);
                             }
                         }
                         // Record the fact that we filled in an extra register slot
@@ -17823,7 +17823,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                             fieldSize,
                                             regTmp,
                                             compiler->lvaOutgoingArgSpaceVar,
-                                            argOffset + fieldArgOffset);
+                                            fieldArgOffset);
                     // We overwrote "regTmp", so erase any previous value we recorded that it contained.
                     regTracker.rsTrackRegTrash(regTmp);
 
@@ -17838,7 +17838,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                                 fieldSize,
                                                 regTmp,
                                                 compiler->lvaOutgoingArgSpaceVar,
-                                                argOffset + fieldArgOffset + TARGET_POINTER_SIZE);
+                                                fieldArgOffset + TARGET_POINTER_SIZE);
                         // Record the fact that we filled in an extra stack slot
                         filledExtraSlot = true;
                     }
@@ -17885,8 +17885,6 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                 {
                     if (curRegNum != MAX_REG_ARG)
                     {
-                        // We are going to write to the lvaPromotedStructAssemblyScratchVar, at the offset
-                        // of this field within the current slot.
                         noway_assert(compiler->lvaPromotedStructAssemblyScratchVar != BAD_VAR_NUM);
 
                         getEmitter()->emitIns_S_R(ins_Store(fieldTypeForInstr),
@@ -17902,7 +17900,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                                 fieldSize,
                                                 fieldVarDsc->lvRegNum,
                                                 compiler->lvaOutgoingArgSpaceVar,
-                                                argOffset + fieldArgOffset);
+                                                fieldArgOffset);
                     }
                 }
                 else
@@ -17922,8 +17920,6 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
 
                     if (curRegNum != MAX_REG_ARG)
                     {                    
-                        // We are going to write to the lvaPromotedStructAssemblyScratchVar, at the offset
-                        // of this field within the current slot.
                         noway_assert(compiler->lvaPromotedStructAssemblyScratchVar != BAD_VAR_NUM);
 
                         getEmitter()->emitIns_S_R(ins_Store(fieldTypeForInstr),
@@ -17938,7 +17934,7 @@ bool CodeGen::genFillSlotFromPromotedStruct(GenTreePtr        arg,
                                                 fieldSize,
                                                 regTmp,
                                                 compiler->lvaOutgoingArgSpaceVar,
-                                                argOffset + fieldArgOffset);
+                                                fieldArgOffset);
                     }
                 }
                 // Go to the next field.
diff --git a/src/jit/compiler.h b/src/jit/compiler.h
index c96defb9a7..324a19de62 100644
--- a/src/jit/compiler.h
+++ b/src/jit/compiler.h
@@ -6824,7 +6824,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.
-    static GenTreePtr   fgIsIndirOfAddrOfLocal(GenTreePtr tree);
+    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/jit/flowgraph.cpp b/src/jit/flowgraph.cpp
index 928b2b341c..83c9ad5023 100644
--- a/src/jit/flowgraph.cpp
+++ b/src/jit/flowgraph.cpp
@@ -6756,7 +6756,7 @@ bool                Compiler::fgIsCommaThrow(GenTreePtr tree,
     return false;
 }
 
-// static
+
 GenTreePtr          Compiler::fgIsIndirOfAddrOfLocal(GenTreePtr tree)
 {
     GenTreePtr res = nullptr;
@@ -20592,7 +20592,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 [%06d]: ", dspTreeID(tree));
+        printf("Missing flags on tree [%X]: ", tree);
         GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
@@ -20600,7 +20600,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 [%06d]: ", dspTreeID(tree));
+        printf("Missing flags on tree [%X]: ", tree);
         GenTree::gtDispFlags(chkFlags & ~treeFlags, GTF_DEBUG_NONE);
         printf("\n");
         gtDispTree(tree);
@@ -22330,11 +22330,9 @@ GenTreePtr      Compiler::fgInlinePrependStatements(InlineInfo* inlineInfo)
                     !(argSingleUseNode->gtFlags & GTF_VAR_CLONED) &&
                     !inlArgInfo[argNum].argHasLdargaOp)
                 {
-                    // 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);
+                    /* Change the temp in-place to the actual argument */
+
+                    argSingleUseNode->CopyFrom(inlArgInfo[argNum].argNode, this);
                     continue;
                 }
                 else
diff --git a/src/jit/gentree.cpp b/src/jit/gentree.cpp
index 54e294847b..30f25b2c57 100644
--- a/src/jit/gentree.cpp
+++ b/src/jit/gentree.cpp
@@ -5313,14 +5313,12 @@ 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:
@@ -6047,16 +6045,7 @@ GenTreeObj* Compiler::gtNewObjNode(CORINFO_CLASS_HANDLE structHnd, GenTree* addr
 {
     var_types nodeType   = impNormStructType(structHnd);
     assert(varTypeIsStruct(nodeType));
-    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) &&
-        !lvaTable[lclNode->gtLclNum].lvAddrExposed)
-    {
-        objNode->gtFlags &= ~GTF_GLOB_REF;
-    }
-    return objNode;
+    return new (this, GT_OBJ) GenTreeObj(nodeType, addr, structHnd);
 }
 
 // Creates a new CpObj node.
@@ -14278,8 +14267,6 @@ bool FieldSeqNode::IsPseudoField()
 #ifdef FEATURE_SIMD
 GenTreeSIMD* Compiler::gtNewSIMDNode(var_types type, GenTreePtr op1, 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->OperGet() == GT_LCL_VAR)
     {
@@ -14293,8 +14280,6 @@ 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/jit/gentree.h b/src/jit/gentree.h
index 85202ec533..716d38553e 100644
--- a/src/jit/gentree.h
+++ b/src/jit/gentree.h
@@ -3329,8 +3329,7 @@ struct GenTreeObj: public GenTreeUnOp
         GenTreeUnOp(GT_OBJ, type, addr),
         gtClass(cls)
         {
-            // By default, an OBJ is assumed to be a global reference.
-            gtFlags |= GTF_GLOB_REF;
+            gtFlags |= GTF_GLOB_REF; // An Obj is always a global reference.
         }
 
 #if DEBUGGABLE_GENTREE
diff --git a/src/jit/importer.cpp b/src/jit/importer.cpp
index 935d709ce5..8a07d9214c 100644
--- a/src/jit/importer.cpp
+++ b/src/jit/importer.cpp
@@ -1498,7 +1498,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)
@@ -1512,10 +1512,9 @@ 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.
-        if (!lvaIsImplicitByRefLocal(structVal->AsLclVarCommon()->gtLclNum))
-        {
-            structObj->gtFlags &= ~GTF_GLOB_REF;
-        }
+        //
+        // TODO-CQ: Clear the GTF_GLOB_REF flag on structObj as well
+        //          but this needs additional work when inlining.
     }
     else  
     {
@@ -16799,17 +16798,15 @@ 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 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 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 (!varTypeIsStruct(lclTyp) &&
-                (!inlArgInfo[lclNum].argHasSideEff) &&
+            if ((!inlArgInfo[lclNum].argHasSideEff) &&
                 (!inlArgInfo[lclNum].argHasGlobRef))
             {
                 /* Get a *LARGE* LCL_VAR node */
diff --git a/src/jit/morph.cpp b/src/jit/morph.cpp
index 23709c3041..e24e8ae7cd 100755
--- a/src/jit/morph.cpp
+++ b/src/jit/morph.cpp
@@ -2067,9 +2067,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.
-        // gtNewObjNode will set the GTF_EXCEPT flag if this is not a local stack object.
+        // Get a new Obj node temp to use it as a call argument
         arg = gtNewObjNode(lvaGetStruct(tmpVarNum), arg);
+        arg->gtFlags |= GTF_EXCEPT;
 
 #endif  // not (_TARGET_AMD64_ or _TARGET_ARM64_)
 
@@ -3522,8 +3522,7 @@ GenTreeCall* Compiler::fgMorphArgs(GenTreeCall* callNode)
                             // we must copyblk to a temp before doing the obj to avoid
                             // the obj reading memory past the end of the valuetype
 #if defined(_TARGET_X86_) && !defined(LEGACY_BACKEND)
-                            // TODO-X86-CQ: [1091733] We should only be copying when roundupSize > originalSize.
-                            // See also the TODO-CQ just before the call to fgMakeOutgoingStructArgCopy().
+                        // TODO-X86-CQ: [1091733] Revisit for small structs, we should use push instruction
                             copyBlkClass = objClass;
                             size = roundupSize / TARGET_POINTER_SIZE;   // Normalize size to number of pointer sized items
 #else // !defined(_TARGET_X86_) || defined(LEGACY_BACKEND)
@@ -4238,6 +4237,8 @@ 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;
                     }
                 }
             }
@@ -8069,19 +8070,17 @@ ONE_SIMPLE_ASG:
             }
         }
 
-        // Indirect the dest node.
+        /* Indirect the dest node */
 
         dest = gtNewOperNode(GT_IND, type, dest);
 
-        // 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.
+        /* 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 (!fgIsIndirOfAddrOfLocal(dest))
-        {
-            dest->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
-        }
+        dest->gtFlags |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
 
 _DoneDest:;
 
@@ -8129,19 +8128,10 @@ _DoneDest:;
                 }
             }
 
-            // Indirect the src node.
+            /* Indirect the src node */
 
             src  = gtNewOperNode(GT_IND, type, src);
-
-            // 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);
-            }
+            src->gtFlags     |= (GTF_EXCEPT | GTF_GLOB_REF | GTF_IND_TGTANYWHERE);
 
 _DoneSrc:;
 
@@ -11452,16 +11442,6 @@ 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/jit/regalloc.cpp b/src/jit/regalloc.cpp
index 36c0dcdf13..4ae9009b6b 100644
--- a/src/jit/regalloc.cpp
+++ b/src/jit/regalloc.cpp
@@ -4683,7 +4683,7 @@ HANDLE_SHIFT_COUNT:
                     }
                 }
 
-                if ((promotedStructLocal != NULL) && (curArgMask != RBM_NONE))
+                if (promotedStructLocal != NULL)
                 {
                     // 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 that the second arg to be evaluated