From 46487aa135baa302b8fed28d03de25baaa5c9e2b Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Fri, 15 Jul 2016 12:29:29 -0700 Subject: [PATCH] More Struct-related Refactorings - Make GT_OBJ and GT_NULLCHECK derive from GenTreeIndir, as they are also indirections. - Extract the ordering-related methods from gentree and optcse, as they will need to be used slightly differently for struct assignments. - Make fgMorphImplicitByRefArgs take a pointer to the tree, as it may need to replace the tree with one that a simple CopyFrom() will not work for. - In gtSetEvalOrder(), in the addressing mode code, replace "adr" with "base" and "op1" with "addr", as the existing names are quite confusing. - Other minor refactorings to reduce future diffs. Commit migrated from https://github.com/dotnet/coreclr/commit/c4ac0d992c883342dd6dff3b1da2565bc0f7b0a1 --- src/coreclr/src/jit/compiler.h | 17 ++- src/coreclr/src/jit/flowgraph.cpp | 16 ++- src/coreclr/src/jit/gentree.cpp | 241 ++++++++++++++++++++------------------ src/coreclr/src/jit/gentree.h | 42 ++++--- src/coreclr/src/jit/gtstructs.h | 2 +- src/coreclr/src/jit/morph.cpp | 66 ++++++----- src/coreclr/src/jit/optcse.cpp | 47 ++++++-- 7 files changed, 247 insertions(+), 184 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index 324a19d..033837e 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -2006,12 +2006,16 @@ public: GenTreePtr gtWalkOpEffectiveVal(GenTreePtr op); #endif - void gtPrepareCost (GenTree * tree); - bool gtIsLikelyRegVar(GenTree * tree); + void gtPrepareCost (GenTree* tree); + bool gtIsLikelyRegVar(GenTree* tree); unsigned gtSetEvalOrderAndRestoreFPstkLevel(GenTree * tree); - unsigned gtSetEvalOrder (GenTree * tree); + // Returns true iff the secondNode can be swapped with firstNode. + bool gtCanSwapOrder (GenTree* firstNode, + GenTree* secondNode); + + unsigned gtSetEvalOrder (GenTree* tree); #if FEATURE_STACK_FP_X87 bool gtFPstLvlRedo; @@ -4772,7 +4776,7 @@ private: fgWalkResult fgMorphStructField(GenTreePtr tree, fgWalkData *fgWalkPre); fgWalkResult fgMorphLocalField(GenTreePtr tree, fgWalkData *fgWalkPre); void fgMarkImplicitByRefArgs(); - bool fgMorphImplicitByRefArgs(GenTreePtr tree, fgWalkData *fgWalkPre); + bool fgMorphImplicitByRefArgs(GenTree** pTree, fgWalkData *fgWalkPre); static fgWalkPreFn fgMarkAddrTakenLocalsPreCB; static fgWalkPostFn fgMarkAddrTakenLocalsPostCB; void fgMarkAddressExposedLocals(); @@ -5405,8 +5409,9 @@ protected : // void optCSE_GetMaskData (GenTreePtr tree, optCSE_MaskData* pMaskData); - // Given a binary tree node return true if it is safe to swap the order of evaluation for op1 and op2 - bool optCSE_canSwap (GenTreePtr tree); + // Given a binary tree node return true if it is safe to swap the order of evaluation for op1 and op2. + bool optCSE_canSwap(GenTree* firstNode, GenTree* secondNode); + bool optCSE_canSwap(GenTree* tree); static fgWalkPostFn optPropagateNonCSE; static fgWalkPreFn optHasNonCSEChild; diff --git a/src/coreclr/src/jit/flowgraph.cpp b/src/coreclr/src/jit/flowgraph.cpp index ed2e6f2..fa2c396 100644 --- a/src/coreclr/src/jit/flowgraph.cpp +++ b/src/coreclr/src/jit/flowgraph.cpp @@ -6771,13 +6771,23 @@ bool Compiler::fgIsCommaThrow(GenTreePtr tree, return false; } - +//------------------------------------------------------------------------ +// fgIsIndirOfAddrOfLocal: Determine whether "tree" is an indirection of a local. +// +// Arguments: +// tree - The tree node under consideration +// +// Return Value: +// If "tree" is a indirection (GT_IND, GT_BLK, or GT_OBJ) whose arg is an ADDR, +// whose arg in turn is a LCL_VAR, return that LCL_VAR node, else nullptr. +// +// static GenTreePtr Compiler::fgIsIndirOfAddrOfLocal(GenTreePtr tree) { GenTreePtr res = nullptr; - if (tree->OperGet() == GT_OBJ || tree->OperIsIndir()) + if (tree->OperIsIndir()) { - GenTreePtr addr = tree->gtOp.gtOp1; + GenTreePtr addr = tree->AsIndir()->Addr(); // Post rationalization, we can have Indir(Lea(..) trees. Therefore to recognize // Indir of addr of a local, skip over Lea in Indir(Lea(base, index, scale, offset)) diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 30f25b2..67440f2 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -2978,6 +2978,71 @@ bool Compiler::gtIsLikelyRegVar(GenTree * tree) return true; } +//------------------------------------------------------------------------ +// gtCanSwapOrder: Returns true iff the secondNode can be swapped with firstNode. +// +// Arguments: +// firstNode - An operand of a tree that can have GTF_REVERSE_OPS set. +// secondNode - The other operand of the tree. +// +// Return Value: +// Returns a boolean indicating whether it is safe to reverse the execution +// order of the two trees, considering any exception, global effects, or +// ordering constraints. +// +bool +Compiler::gtCanSwapOrder(GenTree* firstNode, GenTree* secondNode) +{ + // Relative of order of global / side effects can't be swapped. + + bool canSwap = true; + + if (optValnumCSE_phase) + { + canSwap = optCSE_canSwap(firstNode, secondNode); + } + + // We cannot swap in the presence of special side effects such as GT_CATCH_ARG. + + if (canSwap && + (firstNode->gtFlags & GTF_ORDER_SIDEEFF)) + { + canSwap = false; + } + + // When strict side effect order is disabled we allow GTF_REVERSE_OPS to be set + // when one or both sides contains a GTF_CALL or GTF_EXCEPT. + // Currently only the C and C++ languages allow non strict side effect order. + + unsigned strictEffects = GTF_GLOB_EFFECT; + + if (canSwap && + (firstNode->gtFlags & strictEffects)) + { + // op1 has side efects that can't be reordered. + // Check for some special cases where we still may be able to swap. + + if (secondNode->gtFlags & strictEffects) + { + // op2 has also has non reorderable side effects - can't swap. + canSwap = false; + } + else + { + // No side effects in op2 - we can swap iff op1 has no way of modifying op2, + // i.e. through byref assignments or calls or op2 is a constant. + + if (firstNode->gtFlags & strictEffects & GTF_PERSISTENT_SIDE_EFFECTS) + { + // We have to be conservative - can swap iff op2 is constant. + if (!secondNode->OperIsConst()) + canSwap = false; + } + } + } + return canSwap; +} + /***************************************************************************** * * Given a tree, figure out the order in which its sub-operands should be @@ -3543,17 +3608,18 @@ COMMON_CNS: unsigned mul; #endif unsigned cns; - GenTreePtr adr; + GenTreePtr base; GenTreePtr idx; /* See if we can form a complex addressing mode? */ - if (codeGen->genCreateAddrMode(op1, // address + GenTreePtr addr = op1; + if (codeGen->genCreateAddrMode(addr, // address 0, // mode false, // fold RBM_NONE, // reg mask &rev, // reverse ops - &adr, // base addr + &base, // base addr &idx, // index val #if SCALED_ADDR_MODES &mul, // scaling @@ -3564,17 +3630,17 @@ COMMON_CNS: // We can form a complex addressing mode, so mark each of the interior // nodes with GTF_ADDRMODE_NO_CSE and calculate a more accurate cost. - op1->gtFlags |= GTF_ADDRMODE_NO_CSE; + addr->gtFlags |= GTF_ADDRMODE_NO_CSE; #ifdef _TARGET_XARCH_ // addrmodeCount is the count of items that we used to form // an addressing mode. The maximum value is 4 when we have - // all of these: { adr, idx, cns, mul } + // all of these: { base, idx, cns, mul } // unsigned addrmodeCount = 0; - if (adr) + if (base) { - costEx += adr->gtCostEx; - costSz += adr->gtCostSz; + costEx += base->gtCostEx; + costSz += base->gtCostSz; addrmodeCount++; } @@ -3604,7 +3670,7 @@ COMMON_CNS: // / \ -- // GT_ADD 'cns' -- reduce this interior GT_ADD by (-2,-2) // / \ -- - // 'adr' GT_LSL -- reduce this interior GT_LSL by (-1,-1) + // 'base' GT_LSL -- reduce this interior GT_LSL by (-1,-1) // / \ -- // 'idx' 'mul' // @@ -3614,7 +3680,7 @@ COMMON_CNS: // addrmodeCount--; - GenTreePtr tmp = op1; + GenTreePtr tmp = addr; while (addrmodeCount > 0) { // decrement the gtCosts for the interior GT_ADD or GT_LSH node by the remaining addrmodeCount @@ -3627,7 +3693,7 @@ COMMON_CNS: GenTreePtr tmpOp2 = tmp->gtGetOp2(); assert(tmpOp2 != nullptr); - if ((tmpOp1 != adr) && (tmpOp1->OperGet() == GT_ADD)) + if ((tmpOp1 != base) && (tmpOp1->OperGet() == GT_ADD)) { tmp = tmpOp1; } @@ -3653,11 +3719,11 @@ COMMON_CNS: } } #elif defined _TARGET_ARM_ - if (adr) + if (base) { - costEx += adr->gtCostEx; - costSz += adr->gtCostSz; - if ((adr->gtOper == GT_LCL_VAR) && + costEx += base->gtCostEx; + costSz += base->gtCostSz; + if ((base->gtOper == GT_LCL_VAR) && ((idx==NULL) || (cns==0))) { costSz -= 1; @@ -3691,10 +3757,10 @@ COMMON_CNS: } } #elif defined _TARGET_ARM64_ - if (adr) + if (base) { - costEx += adr->gtCostEx; - costSz += adr->gtCostSz; + costEx += base->gtCostEx; + costSz += base->gtCostSz; } if (idx) @@ -3715,62 +3781,62 @@ COMMON_CNS: #error "Unknown _TARGET_" #endif - assert(op1->gtOper == GT_ADD); - assert(!op1->gtOverflow()); + assert(addr->gtOper == GT_ADD); + assert(!addr->gtOverflow()); assert(op2 == NULL); assert(mul != 1); // If we have an addressing mode, we have one of: - // [adr + cns] - // [ idx * mul ] // mul >= 2, else we would use adr instead of idx - // [ idx * mul + cns] // mul >= 2, else we would use adr instead of idx - // [adr + idx * mul ] // mul can be 0, 2, 4, or 8 - // [adr + idx * mul + cns] // mul can be 0, 2, 4, or 8 + // [base + cns] + // [ idx * mul ] // mul >= 2, else we would use base instead of idx + // [ idx * mul + cns] // mul >= 2, else we would use base instead of idx + // [base + idx * mul ] // mul can be 0, 2, 4, or 8 + // [base + idx * mul + cns] // mul can be 0, 2, 4, or 8 // Note that mul == 0 is semantically equivalent to mul == 1. // Note that cns can be zero. #if SCALED_ADDR_MODES - assert((adr != NULL) || (idx != NULL && mul >= 2)); + assert((base != NULL) || (idx != NULL && mul >= 2)); #else - assert(adr != NULL); + assert(base != NULL); #endif - INDEBUG(GenTreePtr op1Save = op1); + INDEBUG(GenTreePtr op1Save = addr); - /* Walk op1 looking for non-overflow GT_ADDs */ - gtWalkOp(&op1, &op2, adr, false); + /* Walk addr looking for non-overflow GT_ADDs */ + gtWalkOp(&addr, &op2, base, false); - // op1 and op2 are now children of the root GT_ADD of the addressing mode - assert(op1 != op1Save); + // addr and op2 are now children of the root GT_ADD of the addressing mode + assert(addr != op1Save); assert(op2 != NULL); - /* Walk op1 looking for non-overflow GT_ADDs of constants */ - gtWalkOp(&op1, &op2, NULL, true); + /* Walk addr looking for non-overflow GT_ADDs of constants */ + gtWalkOp(&addr, &op2, NULL, true); // TODO-Cleanup: It seems very strange that we might walk down op2 now, even though the prior // call to gtWalkOp() may have altered op2. /* Walk op2 looking for non-overflow GT_ADDs of constants */ - gtWalkOp(&op2, &op1, NULL, true); + gtWalkOp(&op2, &addr, NULL, true); // OK we are done walking the tree - // Now assert that op1 and op2 correspond with adr and idx + // Now assert that addr and op2 correspond with base and idx // in one of the several acceptable ways. - // Note that sometimes op1/op2 is equal to idx/adr - // and other times op1/op2 is a GT_COMMA node with - // an effective value that is idx/adr + // Note that sometimes addr/op2 is equal to idx/base + // and other times addr/op2 is a GT_COMMA node with + // an effective value that is idx/base if (mul > 1) { - if ((op1 != adr) && (op1->gtOper == GT_LSH)) + if ((addr != base) && (addr->gtOper == GT_LSH)) { - op1->gtFlags |= GTF_ADDRMODE_NO_CSE; - if (op1->gtOp.gtOp1->gtOper == GT_MUL) + addr->gtFlags |= GTF_ADDRMODE_NO_CSE; + if (addr->gtOp.gtOp1->gtOper == GT_MUL) { - op1->gtOp.gtOp1->gtFlags |= GTF_ADDRMODE_NO_CSE; + addr->gtOp.gtOp1->gtFlags |= GTF_ADDRMODE_NO_CSE; } - assert((adr == NULL) || (op2 == adr) || (op2->gtEffectiveVal() == adr->gtEffectiveVal()) || - (gtWalkOpEffectiveVal(op2) == gtWalkOpEffectiveVal(adr))); + assert((base == NULL) || (op2 == base) || (op2->gtEffectiveVal() == base->gtEffectiveVal()) || + (gtWalkOpEffectiveVal(op2) == gtWalkOpEffectiveVal(base))); } else { @@ -3785,7 +3851,7 @@ COMMON_CNS: op2op1->gtFlags |= GTF_ADDRMODE_NO_CSE; op2op1 = op2op1->gtOp.gtOp1; } - assert(op1->gtEffectiveVal() == adr); + assert(addr->gtEffectiveVal() == base); assert(op2op1 == idx); } } @@ -3793,24 +3859,24 @@ COMMON_CNS: { assert(mul == 0); - if ((op1 == idx) || (op1->gtEffectiveVal() == idx)) + if ((addr == idx) || (addr->gtEffectiveVal() == idx)) { if (idx != NULL) { - if ((op1->gtOper == GT_MUL) || (op1->gtOper == GT_LSH)) + if ((addr->gtOper == GT_MUL) || (addr->gtOper == GT_LSH)) { - if ((op1->gtOp.gtOp1->gtOper == GT_NOP) || - (op1->gtOp.gtOp1->gtOper == GT_MUL && op1->gtOp.gtOp1->gtOp.gtOp1->gtOper == GT_NOP)) + if ((addr->gtOp.gtOp1->gtOper == GT_NOP) || + (addr->gtOp.gtOp1->gtOper == GT_MUL && addr->gtOp.gtOp1->gtOp.gtOp1->gtOper == GT_NOP)) { - op1->gtFlags |= GTF_ADDRMODE_NO_CSE; - if (op1->gtOp.gtOp1->gtOper == GT_MUL) - op1->gtOp.gtOp1->gtFlags |= GTF_ADDRMODE_NO_CSE; + addr->gtFlags |= GTF_ADDRMODE_NO_CSE; + if (addr->gtOp.gtOp1->gtOper == GT_MUL) + addr->gtOp.gtOp1->gtFlags |= GTF_ADDRMODE_NO_CSE; } } } - assert((op2 == adr) || (op2->gtEffectiveVal() == adr)); + assert((op2 == base) || (op2->gtEffectiveVal() == base)); } - else if ((op1 == adr) || (op1->gtEffectiveVal() == adr)) + else if ((addr == base) || (addr->gtEffectiveVal() == base)) { if (idx != NULL) { @@ -3831,7 +3897,7 @@ COMMON_CNS: } else { - // op1 isn't adr or idx. Is this possible? Or should there be an assert? + // addr isn't base or idx. Is this possible? Or should there be an assert? } } goto DONE; @@ -4302,60 +4368,7 @@ COMMON_CNS: if (tryToSwap) { - /* Relative of order of global / side effects can't be swapped */ - - bool canSwap = true; - - if (optValnumCSE_phase) - { - canSwap = optCSE_canSwap(tree); - } - - /* We cannot swap in the presence of special side effects such as GT_CATCH_ARG */ - - if (canSwap && - (opA->gtFlags & GTF_ORDER_SIDEEFF)) - { - canSwap = false; - } - - /* When strict side effect order is disabled we allow - * GTF_REVERSE_OPS to be set when one or both sides contains - * a GTF_CALL or GTF_EXCEPT. - * Currently only the C and C++ languages - * allow non strict side effect order - */ - unsigned strictEffects = GTF_GLOB_EFFECT; - - if (canSwap && - (opA->gtFlags & strictEffects)) - { - /* op1 has side efects, that can't be reordered. - * Check for some special cases where we still - * may be able to swap - */ - - if (opB->gtFlags & strictEffects) - { - /* op2 has also has non reorderable side effects - can't swap */ - canSwap = false; - } - else - { - /* No side effects in op2 - we can swap iff - * op1 has no way of modifying op2, - * i.e. through byref assignments or calls - * unless op2 is a constant - */ - - if (opA->gtFlags & strictEffects & GTF_PERSISTENT_SIDE_EFFECTS) - { - /* We have to be conservative - can swap iff op2 is constant */ - if (!opB->OperIsConst()) - canSwap = false; - } - } - } + bool canSwap = gtCanSwapOrder(opA, opB); if (canSwap) { @@ -12191,7 +12204,7 @@ void Compiler::gtExtractSideEffList(GenTreePtr expr, GenTreePtr * // Special case - GT_ADDR of GT_IND nodes of TYP_STRUCT // have to be kept together - if (oper == GT_ADDR && op1->gtOper == GT_IND && op1->gtType == TYP_STRUCT) + if (oper == GT_ADDR && op1->OperIsIndir() && op1->gtType == TYP_STRUCT) { *pList = gtBuildCommaList(*pList, expr); @@ -13388,7 +13401,7 @@ bool GenTree::isContainedIndir() const bool GenTree::isIndirAddrMode() { - return isIndir() && gtOp.gtOp1->OperIsAddrMode() && gtOp.gtOp1->isContained(); + return isIndir() && AsIndir()->Addr()->OperIsAddrMode() && AsIndir()->Addr()->isContained(); } bool GenTree::isIndir() const @@ -14041,7 +14054,7 @@ void GenTree::ParseArrayAddressWork(Compiler* comp, ssize_t inputMul, GenTreePtr bool GenTree::ParseArrayElemForm(Compiler* comp, ArrayInfo* arrayInfo, FieldSeqNode** pFldSeq) { - if (OperGet() == GT_IND) + if (OperIsIndir()) { if (gtFlags & GTF_IND_ARR_INDEX) { @@ -14051,7 +14064,7 @@ bool GenTree::ParseArrayElemForm(Compiler* comp, ArrayInfo* arrayInfo, FieldSeqN } // Otherwise... - GenTreePtr addr = gtOp.gtOp1; + GenTreePtr addr = AsIndir()->Addr(); return addr->ParseArrayElemAddrForm(comp, arrayInfo, pFldSeq); } else diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 716d385..b9b97ef 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -1200,7 +1200,7 @@ public: static bool OperIsIndir(genTreeOps gtOper) { - return gtOper == GT_IND || gtOper == GT_STOREIND || gtOper == GT_NULLCHECK; + return gtOper == GT_IND || gtOper == GT_STOREIND || gtOper == GT_NULLCHECK || gtOper == GT_OBJ; } bool OperIsIndir() const @@ -3316,27 +3316,6 @@ protected: #endif // DEBUGGABLE_GENTREE }; -// gtObj -- 'object' (GT_OBJ). */ - -struct GenTreeObj: public GenTreeUnOp -{ - // The address of the block. - GenTreePtr& Addr() { return gtOp1; } - - CORINFO_CLASS_HANDLE gtClass; // the class of the object - - GenTreeObj(var_types type, GenTreePtr addr, CORINFO_CLASS_HANDLE cls) : - GenTreeUnOp(GT_OBJ, type, addr), - gtClass(cls) - { - gtFlags |= GTF_GLOB_REF; // An Obj is always a global reference. - } - -#if DEBUGGABLE_GENTREE - GenTreeObj() : GenTreeUnOp() {} -#endif -}; - // Represents a CpObj MSIL Node. struct GenTreeCpObj : public GenTreeBlkOp { @@ -3545,6 +3524,25 @@ protected: #endif }; +// gtObj -- 'object' (GT_OBJ). */ + +struct GenTreeObj: public GenTreeIndir +{ + CORINFO_CLASS_HANDLE gtClass; // the class of the object + + GenTreeObj(var_types type, GenTreePtr addr, CORINFO_CLASS_HANDLE cls) : + GenTreeIndir(GT_OBJ, type, addr, nullptr), + gtClass(cls) + { + // By default, an OBJ is assumed to be a global reference. + gtFlags |= GTF_GLOB_REF; + } + +#if DEBUGGABLE_GENTREE + GenTreeObj() : GenTreeIndir() {} +#endif +}; + // Read-modify-write status of a RMW memory op rooted at a storeInd enum RMWStatus { STOREIND_RMW_STATUS_UNKNOWN, // RMW status of storeInd unknown diff --git a/src/coreclr/src/jit/gtstructs.h b/src/coreclr/src/jit/gtstructs.h index 2f0b3a3..06c5b98 100644 --- a/src/coreclr/src/jit/gtstructs.h +++ b/src/coreclr/src/jit/gtstructs.h @@ -90,7 +90,7 @@ GTSTRUCT_1(AddrMode , GT_LEA) GTSTRUCT_1(Qmark , GT_QMARK) GTSTRUCT_1(PhiArg , GT_PHI_ARG) GTSTRUCT_1(StoreInd , GT_STOREIND) -GTSTRUCT_2(Indir , GT_STOREIND, GT_IND) +GTSTRUCT_N(Indir , GT_STOREIND, GT_IND, GT_NULLCHECK, GT_OBJ) GTSTRUCT_1(PutArgStk , GT_PUTARG_STK) GTSTRUCT_1(PhysReg , GT_PHYSREG) GTSTRUCT_3(BlkOp , GT_COPYBLK, GT_INITBLK, GT_COPYOBJ) diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index e24e8ae..95d25e3 100755 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -4795,29 +4795,30 @@ Compiler::fgMakeOutgoingStructArgCopy(GenTreeCall* call, // See if we need to insert a copy at all // Case 1: don't need a copy if it is the last use of a local. We can't determine that all of the time // but if there is only one use and no loops, the use must be last. - if (argx->gtOper == GT_OBJ) + GenTreeLclVarCommon* lcl = nullptr; + if ((argx->OperGet() == GT_OBJ) && argx->AsObj()->Addr()->OperIsLocal()) + { + lcl = argx->AsObj()->Addr()->AsLclVarCommon(); + } + if (lcl != nullptr) { - GenTree* lcl = argx->gtOp.gtOp1; - if (lcl->OperIsLocal()) + unsigned varNum = lcl->AsLclVarCommon()->GetLclNum(); + if (lvaIsImplicitByRefLocal(varNum)) { - unsigned varNum = lcl->AsLclVarCommon()->GetLclNum(); - if (lvaIsImplicitByRefLocal(varNum)) + LclVarDsc* varDsc = &lvaTable[varNum]; + // JIT_TailCall helper has an implicit assumption that all tail call arguments live + // on the caller's frame. If an argument lives on the caller caller's frame, it may get + // overwritten if that frame is reused for the tail call. Therefore, we should always copy + // struct parameters if they are passed as arguments to a tail call. + if (!call->IsTailCallViaHelper() && (varDsc->lvRefCnt == 1) && !fgMightHaveLoop()) { - LclVarDsc* varDsc = &lvaTable[varNum]; - // JIT_TailCall helper has an implicit assumption that all tail call arguments live - // on the caller's frame. If an argument lives on the caller caller's frame, it may get - // overwritten if that frame is reused for the tail call. Therefore, we should always copy - // struct parameters if they are passed as arguments to a tail call. - if (!call->IsTailCallViaHelper() && (varDsc->lvRefCnt == 1) && !fgMightHaveLoop()) - { - varDsc->lvRefCnt = 0; - args->gtOp.gtOp1 = lcl; - fgArgTabEntryPtr fp = Compiler::gtArgEntryByNode(call, argx); - fp->node = lcl; + varDsc->lvRefCnt = 0; + args->gtOp.gtOp1 = lcl; + fgArgTabEntryPtr fp = Compiler::gtArgEntryByNode(call, argx); + fp->node = lcl; - JITDUMP("did not have to make outgoing copy for V%2d", varNum); - return; - } + JITDUMP("did not have to make outgoing copy for V%2d", varNum); + return; } } } @@ -5912,13 +5913,14 @@ GenTreePtr Compiler::fgMorphField(GenTreePtr tree, MorphAddrContext* ma lclNum = objRef->gtLclVarCommon.gtLclNum; } - // Create the "nullchk" node - nullchk = gtNewOperNode(GT_NULLCHECK, - TYP_BYTE, // Make it TYP_BYTE so we only deference it for 1 byte. - gtNewLclvNode(lclNum, objRefType)); + // Create the "nullchk" node. + // Make it TYP_BYTE so we only deference it for 1 byte. + GenTreePtr lclVar = gtNewLclvNode(lclNum, objRefType); + nullchk = new(this, GT_NULLCHECK) GenTreeIndir(GT_NULLCHECK, TYP_BYTE, lclVar, nullptr); + nullchk->gtFlags |= GTF_DONT_CSE; // Don't try to create a CSE for these TYP_BYTE indirections - /* An indirection will cause a GPF if the address is null */ + // An indirection will cause a GPF if the address is null. nullchk->gtFlags |= GTF_EXCEPT; if (asg) @@ -6423,12 +6425,15 @@ bool Compiler::fgCanFastTailCall(GenTreeCall* callee) } // Get the size of the struct and see if it is register passable. + CORINFO_CLASS_HANDLE objClass = nullptr; + if (argx->OperGet() == GT_OBJ) { + objClass = argx->AsObj()->gtClass; #if defined(_TARGET_AMD64_) || defined(_TARGET_ARM64_) unsigned typeSize = 0; - hasMultiByteArgs = !VarTypeIsMultiByteAndCanEnreg(argx->TypeGet(), argx->gtObj.gtClass, &typeSize, false); + hasMultiByteArgs = !VarTypeIsMultiByteAndCanEnreg(argx->TypeGet(), objClass, &typeSize, false); #if defined(FEATURE_UNIX_AMD64_STRUCT_PASSING) || defined(_TARGET_ARM64_) // On System V/arm64 the args could be a 2 eightbyte struct that is passed in two registers. @@ -16166,7 +16171,7 @@ void Compiler::fgMarkImplicitByRefArgs() * Morph irregular parameters * for x64 and ARM64 this means turning them into byrefs, adding extra indirs. */ -bool Compiler::fgMorphImplicitByRefArgs(GenTreePtr tree, fgWalkData* fgWalkPre) +bool Compiler::fgMorphImplicitByRefArgs(GenTreePtr *pTree, fgWalkData* fgWalkPre) { #if !defined(_TARGET_AMD64_) && !defined(_TARGET_ARM64_) @@ -16174,6 +16179,7 @@ bool Compiler::fgMorphImplicitByRefArgs(GenTreePtr tree, fgWalkData* fgWalkPre) #else // _TARGET_AMD64_ || _TARGET_ARM64_ + GenTree* tree = *pTree; assert((tree->gtOper == GT_LCL_VAR) || ((tree->gtOper == GT_ADDR) && (tree->gtOp.gtOp1->gtOper == GT_LCL_VAR))); @@ -16201,6 +16207,9 @@ bool Compiler::fgMorphImplicitByRefArgs(GenTreePtr tree, fgWalkData* fgWalkPre) // We are overloading the lvRefCnt field here because real ref counts have not been set. lclVarDsc->lvRefCnt++; + // This is no longer a def of the lclVar, even if it WAS a def of the struct. + lclVarTree->gtFlags &= ~(GTF_LIVENESS_MASK); + if (isAddr) { // change &X into just plain X @@ -16245,6 +16254,7 @@ bool Compiler::fgMorphImplicitByRefArgs(GenTreePtr tree, fgWalkData* fgWalkPre) #endif // DEBUG } + *pTree = tree; return true; #endif // _TARGET_AMD64_ || _TARGET_ARM64_ @@ -16446,7 +16456,7 @@ Compiler::fgWalkResult Compiler::fgMarkAddrTakenLocalsPreCB(GenTreePtr* pTr // If we have ADDR(lcl), and "lcl" is an implicit byref parameter, fgMorphImplicitByRefArgs will // convert to just "lcl". This is never an address-context use, since the local is already a // byref after this transformation. - if (tree->gtOp.gtOp1->OperGet() == GT_LCL_VAR && comp->fgMorphImplicitByRefArgs(tree, fgWalkPre)) + if (tree->gtOp.gtOp1->OperGet() == GT_LCL_VAR && comp->fgMorphImplicitByRefArgs(pTree, fgWalkPre)) { // Push something to keep the PostCB, which will pop it, happy. axcStack->Push(AXC_None); @@ -16547,7 +16557,7 @@ Compiler::fgWalkResult Compiler::fgMarkAddrTakenLocalsPreCB(GenTreePtr* pTr case GT_LCL_VAR: // On some architectures, some arguments are passed implicitly by reference. // Modify the trees to reflect that, if this local is one of those. - if (comp->fgMorphImplicitByRefArgs(tree, fgWalkPre)) + if (comp->fgMorphImplicitByRefArgs(pTree, fgWalkPre)) { // We can't be in an address context; the ADDR(lcl), where lcl is an implicit byref param, was // handled earlier. (And we can't have added anything to this address, since it was implicit.) diff --git a/src/coreclr/src/jit/optcse.cpp b/src/coreclr/src/jit/optcse.cpp index fa19445..c424e7e 100644 --- a/src/coreclr/src/jit/optcse.cpp +++ b/src/coreclr/src/jit/optcse.cpp @@ -313,17 +313,24 @@ void Compiler::optCSE_GetMaskData(GenTreePtr tree, optCSE_MaskDat } -// Given a binary tree node return true if it is safe to swap the order of evaluation for op1 and op2 -// It only considers the locations of the CSE defs and uses for op1 and op2 to decide this +//------------------------------------------------------------------------ +// optCSE_canSwap: Determine if the execution order of two nodes can be swapped. // -bool Compiler::optCSE_canSwap(GenTreePtr tree) +// Arguments: +// op1 - The first node +// op2 - The second node +// +// Return Value: +// Return true iff it safe to swap the execution order of 'op1' and 'op2', +// considering only the locations of the CSE defs and uses. +// +// Assumptions: +// 'op1' currently occurse before 'op2' in the execution order. +// +bool Compiler::optCSE_canSwap(GenTree* op1, GenTree* op2) { - assert((tree->OperKind() & GTK_SMPOP) != 0); - - GenTreePtr op1 = tree->gtOp.gtOp1; - GenTreePtr op2 = tree->gtGetOp2(); - - assert(op1 != nullptr); // We must have a binary treenode with non-null op1 and op2 + // op1 and op2 must be non-null. + assert(op1 != nullptr); assert(op2 != nullptr); bool canSwap = true; // the default result unless proven otherwise. @@ -341,7 +348,7 @@ bool Compiler::optCSE_canSwap(GenTreePtr tree) } else { - // We also cannot swap if op2 contains a CSE def that is used by op1 + // We also cannot swap if op2 contains a CSE def that is used by op1. if ((op2MaskData.CSE_defMask & op1MaskData.CSE_useMask) != 0) { canSwap = false; @@ -351,6 +358,26 @@ bool Compiler::optCSE_canSwap(GenTreePtr tree) return canSwap; } +//------------------------------------------------------------------------ +// optCSE_canSwap: Determine if the execution order of a node's operands can be swapped. +// +// Arguments: +// tree - The node of interest +// +// Return Value: +// Return true iff it safe to swap the execution order of the operands of 'tree', +// considering only the locations of the CSE defs and uses. +// +bool Compiler::optCSE_canSwap(GenTreePtr tree) +{ + // We must have a binary treenode with non-null op1 and op2 + assert((tree->OperKind() & GTK_SMPOP) != 0); + + GenTreePtr op1 = tree->gtOp.gtOp1; + GenTreePtr op2 = tree->gtGetOp2(); + + return optCSE_canSwap(op1, op2); +} /***************************************************************************** * -- 2.7.4