From d63db2d65ec062f115f616019ae997c87a910368 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Tue, 2 Aug 2016 15:05:10 -0700 Subject: [PATCH] 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 --- src/coreclr/src/jit/codegenlegacy.cpp | 5 ++-- src/coreclr/src/jit/compiler.h | 2 +- src/coreclr/src/jit/flowgraph.cpp | 12 ++++++---- src/coreclr/src/jit/gentree.cpp | 19 ++++++++++++--- src/coreclr/src/jit/importer.cpp | 27 +++++++++++---------- src/coreclr/src/jit/morph.cpp | 45 +++++++++++++++++++++++++---------- src/coreclr/src/jit/regalloc.cpp | 2 +- 7 files changed, 75 insertions(+), 37 deletions(-) diff --git a/src/coreclr/src/jit/codegenlegacy.cpp b/src/coreclr/src/jit/codegenlegacy.cpp index 9f242b6..476f8af 100644 --- 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, ®Tmp); if (filledExtraSlot) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index b0125d9..cf8340a 100644 --- 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 --- 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 --- 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 --- 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 --- 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 --- 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 -- 2.7.4