From 9bf91f8d178e09bb81fb1749a82d588cbe8028cf Mon Sep 17 00:00:00 2001 From: Sergey Andreenko Date: Tue, 17 Dec 2019 08:17:26 -0800 Subject: [PATCH] Revert "Start generating LCL_FLDs in LocalAddressVisitor (#737)" (#973) This reverts commit 4fa163852af62908948e8108a03f4044f96d2bc1. --- src/coreclr/src/jit/compiler.h | 1 - src/coreclr/src/jit/gentree.cpp | 2 +- src/coreclr/src/jit/gentree.h | 49 +++------ src/coreclr/src/jit/morph.cpp | 221 ++++++++-------------------------------- src/coreclr/src/jit/simd.cpp | 24 ----- 5 files changed, 59 insertions(+), 238 deletions(-) diff --git a/src/coreclr/src/jit/compiler.h b/src/coreclr/src/jit/compiler.h index bfc7bba..869267d 100644 --- a/src/coreclr/src/jit/compiler.h +++ b/src/coreclr/src/jit/compiler.h @@ -7955,7 +7955,6 @@ private: void setLclRelatedToSIMDIntrinsic(GenTree* tree); bool areFieldsContiguous(GenTree* op1, GenTree* op2); - bool areLocalFieldsContiguous(GenTreeLclFld* first, GenTreeLclFld* second); bool areArrayElementsContiguous(GenTree* op1, GenTree* op2); bool areArgumentsContiguous(GenTree* op1, GenTree* op2); GenTree* createAddressNodeForSIMDInit(GenTree* tree, unsigned simdSize); diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp index 8c0e992..02fd018e 100644 --- a/src/coreclr/src/jit/gentree.cpp +++ b/src/coreclr/src/jit/gentree.cpp @@ -18009,7 +18009,7 @@ bool FieldSeqNode::IsConstantIndexFieldSeq() return m_fieldHnd == FieldSeqStore::ConstantIndexPseudoField; } -bool FieldSeqNode::IsPseudoField() const +bool FieldSeqNode::IsPseudoField() { return m_fieldHnd == FieldSeqStore::FirstElemPseudoField || m_fieldHnd == FieldSeqStore::ConstantIndexPseudoField; } diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 47c8a38..c88ba80 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -235,23 +235,7 @@ struct FieldSeqNode bool IsConstantIndexFieldSeq(); // returns true when this is the the pseudo #FirstElem field sequence or the pseudo #ConstantIndex field sequence - bool IsPseudoField() const; - - CORINFO_FIELD_HANDLE GetFieldHandle() const - { - assert(!IsPseudoField() && (m_fieldHnd != nullptr)); - return m_fieldHnd; - } - - FieldSeqNode* GetTail() - { - FieldSeqNode* tail = this; - while (tail->m_next != nullptr) - { - tail = tail->m_next; - } - return tail; - } + bool IsPseudoField(); // Make sure this provides methods that allow it to be used as a KeyFuncs type in SimplerHash. static int GetHashCode(FieldSeqNode fsn) @@ -3260,13 +3244,6 @@ struct GenTreeField : public GenTree gtFieldLookup.addr = nullptr; #endif } - - // True if this field is a volatile memory operation. - bool IsVolatile() const - { - return (gtFlags & GTF_FLD_VOLATILE) != 0; - } - #if DEBUGGABLE_GENTREE GenTreeField() : GenTree() { @@ -5061,18 +5038,6 @@ struct GenTreeIndir : public GenTreeOp { } - // True if this indirection is a volatile memory operation. - bool IsVolatile() const - { - return (gtFlags & GTF_IND_VOLATILE) != 0; - } - - // True if this indirection is an unaligned memory operation. - bool IsUnaligned() const - { - return (gtFlags & GTF_IND_UNALIGNED) != 0; - } - #if DEBUGGABLE_GENTREE protected: friend GenTree; @@ -5124,6 +5089,18 @@ public: return (m_layout != nullptr) ? m_layout->GetSize() : 0; } + // True if this BlkOpNode is a volatile memory operation. + bool IsVolatile() const + { + return (gtFlags & GTF_BLK_VOLATILE) != 0; + } + + // True if this BlkOpNode is an unaligned memory operation. + bool IsUnaligned() const + { + return (gtFlags & GTF_BLK_UNALIGNED) != 0; + } + // Instruction selection: during codegen time, what code sequence we will be using // to encode this operation. enum diff --git a/src/coreclr/src/jit/morph.cpp b/src/coreclr/src/jit/morph.cpp index c600733..75ceb05 100644 --- a/src/coreclr/src/jit/morph.cpp +++ b/src/coreclr/src/jit/morph.cpp @@ -5448,23 +5448,18 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) GenTree* arrRef2 = nullptr; // The second copy will be used in array address expression GenTree* index2 = nullptr; - // If the arrRef or index expressions involves an assignment, a call or reads from global memory, - // then we *must* allocate a temporary in which to "localize" those values, to ensure that the - // same values are used in the bounds check and the actual dereference. - // Also we allocate the temporary when the expresion is sufficiently complex/expensive. + // If the arrRef expression involves an assignment, a call or reads from global memory, + // then we *must* allocate a temporary in which to "localize" those values, + // to ensure that the same values are used in the bounds check and the actual + // dereference. + // Also we allocate the temporary when the arrRef is sufficiently complex/expensive. + // Note that if 'arrRef' is a GT_FIELD, it has not yet been morphed so its true + // complexity is not exposed. (Without that condition there are cases of local struct + // fields that were previously, needlessly, marked as GTF_GLOB_REF, and when that was + // fixed, there were some regressions that were mostly ameliorated by adding this condition.) // - // Note that if the expression is a GT_FIELD, it has not yet been morphed so its true complexity is - // not exposed. Without that condition there are cases of local struct fields that were previously, - // needlessly, marked as GTF_GLOB_REF, and when that was fixed, there were some regressions that - // were mostly ameliorated by adding this condition. - // - // Likewise, allocate a temporary if the expression is a GT_LCL_FLD node. These used to be created - // after fgMorphArrayIndex from GT_FIELD trees so this preserves the existing behavior. This is - // perhaps a decision that should be left to CSE but FX diffs show that it is slightly better to - // do this here. - if ((arrRef->gtFlags & (GTF_ASG | GTF_CALL | GTF_GLOB_REF)) || - gtComplexityExceeds(&arrRef, MAX_ARR_COMPLEXITY) || arrRef->OperIs(GT_FIELD, GT_LCL_FLD)) + gtComplexityExceeds(&arrRef, MAX_ARR_COMPLEXITY) || (arrRef->OperGet() == GT_FIELD)) { unsigned arrRefTmpNum = lvaGrabTemp(true DEBUGARG("arr expr")); arrRefDefn = gtNewTempAssign(arrRefTmpNum, arrRef); @@ -5477,10 +5472,16 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree) noway_assert(arrRef2 != nullptr); } + // If the index expression involves an assignment, a call or reads from global memory, + // we *must* allocate a temporary in which to "localize" those values, + // to ensure that the same values are used in the bounds check and the actual + // dereference. + // Also we allocate the temporary when the index is sufficiently complex/expensive. + // if ((index->gtFlags & (GTF_ASG | GTF_CALL | GTF_GLOB_REF)) || gtComplexityExceeds(&index, MAX_ARR_COMPLEXITY) || - index->OperIs(GT_FIELD, GT_LCL_FLD)) + (arrRef->OperGet() == GT_FIELD)) { - unsigned indexTmpNum = lvaGrabTemp(true DEBUGARG("index expr")); + unsigned indexTmpNum = lvaGrabTemp(true DEBUGARG("arr expr")); indexDefn = gtNewTempAssign(indexTmpNum, index); index = gtNewLclvNode(indexTmpNum, index->TypeGet()); index2 = gtNewLclvNode(indexTmpNum, index->TypeGet()); @@ -5758,7 +5759,7 @@ GenTree* Compiler::fgMorphStackArgForVarArgs(unsigned lclNum, var_types varType, // Create a node representing the local pointing to the base of the args GenTree* ptrArg = gtNewOperNode(GT_SUB, TYP_I_IMPL, gtNewLclvNode(lvaVarargsBaseOfStkArgs, TYP_I_IMPL), - gtNewIconNode(varDsc->lvStkOffs - codeGen->intRegState.rsCalleeRegArgCount * REGSIZE_BYTES - + gtNewIconNode(varDsc->lvStkOffs - codeGen->intRegState.rsCalleeRegArgCount * REGSIZE_BYTES + lclOffs)); // Access the argument through the local @@ -8550,14 +8551,9 @@ GenTree* Compiler::fgMorphLeaf(GenTree* tree) const bool forceRemorph = false; return fgMorphLocalVar(tree, forceRemorph); } +#ifdef _TARGET_X86_ else if (tree->gtOper == GT_LCL_FLD) { - if (lvaGetDesc(tree->AsLclFld())->lvAddrExposed) - { - tree->gtFlags |= GTF_GLOB_REF; - } - -#ifdef _TARGET_X86_ if (info.compIsVarArgs) { GenTree* newTree = fgMorphStackArgForVarArgs(tree->AsLclFld()->GetLclNum(), tree->TypeGet(), @@ -8571,8 +8567,8 @@ GenTree* Compiler::fgMorphLeaf(GenTree* tree) return newTree; } } -#endif // _TARGET_X86_ } +#endif // _TARGET_X86_ else if (tree->gtOper == GT_FTN_ADDR) { CORINFO_CONST_LOOKUP addrInfo; @@ -17659,7 +17655,7 @@ class LocalAddressVisitor final : public GenTreeVisitor } //------------------------------------------------------------------------ - // Address: Produce an address value from a GT_LCL_VAR_ADDR node. + // Location: Produce an address value from a GT_LCL_VAR_ADDR node. // // Arguments: // lclVar - a GT_LCL_VAR_ADDR node that defines the address @@ -18251,14 +18247,6 @@ private: { m_compiler->lvaSetVarAddrExposed(varDsc->lvIsStructField ? varDsc->lvParentLcl : val.LclNum()); } - else if (node->OperIs(GT_IND, GT_FIELD)) - { - // TODO-ADDR: This should also work with OBJ and BLK but that typically requires - // struct typed LCL_FLDs which are not yet supported. Also, OBJs that are call - // arguments requires special care - at least because of the current PUTARG_STK - // codegen that requires OBJs. - MorphLocalIndir(val, user); - } } INDEBUG(val.Consume();) @@ -18383,110 +18371,6 @@ private: } //------------------------------------------------------------------------ - // MorphLocalIndir: Change a tree that represents an indirect access to a struct - // variable to a single LCL_FLD node. - // - // Arguments: - // val - a value that represents the local indirection - // user - the indirection's user node - // - void MorphLocalIndir(const Value& val, GenTree* user) - { - assert(val.IsLocation()); - - GenTree* indir = val.Node(); - assert(indir->OperIs(GT_IND, GT_FIELD)); - - if ((indir->OperIs(GT_IND) && indir->AsIndir()->IsVolatile()) || - (indir->OperIs(GT_FIELD) && indir->AsField()->IsVolatile())) - { - return; - } - - if (varTypeIsStruct(indir->TypeGet())) - { - // TODO-ADDR: Skip struct indirs for now, they require struct typed LCL_FLDs. - // Also skip SIMD indirs for now, SIMD typed LCL_FLDs works most of the time - // but there are exceptions - fgMorphFieldAssignToSIMDIntrinsicSet for example. - return; - } - - LclVarDsc* varDsc = m_compiler->lvaGetDesc(val.LclNum()); - - if (varDsc->TypeGet() != TYP_STRUCT) - { - // TODO-ADDR: Skip integral/floating point variables for now, they're more - // complicated to transform. We can always turn an indirect access of such - // a variable into a LCL_FLD but that blocks enregistration so we need to - // detect those case where we can use LCL_VAR instead, perhaps in conjuction - // with CAST and/or BITCAST. - // Also skip SIMD variables for now, fgMorphFieldAssignToSIMDIntrinsicSet and - // others need to be updated to recognize LCL_FLDs. - return; - } - - if (varDsc->lvPromoted || varDsc->lvIsStructField || m_compiler->lvaIsImplicitByRefLocal(val.LclNum())) - { - // TODO-ADDR: For now we ignore promoted and "implict by ref" variables, - // they require additional changes in subsequent phases - // (e.g. fgMorphImplicitByRefArgs does not handle LCL_FLD nodes). - return; - } - - FieldSeqNode* fieldSeq = val.FieldSeq(); - - if ((fieldSeq != nullptr) && (fieldSeq != FieldSeqStore::NotAField())) - { - if (indir->OperIs(GT_IND)) - { - // If we have an IND node and a field sequence then they should have the same type. - // Otherwise it's best to forget the field sequence since the resulting LCL_FLD - // doesn't match a real struct field. Value numbering protects itself from such - // mismatches but there doesn't seem to be any good reason to generate a LCL_FLD - // with a mismatched field sequence only to have to ignore it later. - - if (indir->TypeGet() != - JITtype2varType(m_compiler->info.compCompHnd->getFieldType(fieldSeq->GetTail()->GetFieldHandle()))) - { - fieldSeq = nullptr; - } - } - else if (indir->OperIs(GT_FIELD)) - { - // TODO-ADDR: ObjectAllocator produces FIELD nodes with FirstElemPseudoField as field - // handle so we cannot use FieldSeqNode::GetFieldHandle() because it asserts on such - // handles. ObjectAllocator should be changed to create LCL_FLD nodes directly. - assert(fieldSeq->GetTail()->m_fieldHnd == indir->AsField()->gtFldHnd); - } - } - - indir->ChangeOper(GT_LCL_FLD); - indir->AsLclFld()->SetLclNum(val.LclNum()); - indir->AsLclFld()->SetLclOffs(val.Offset()); - indir->AsLclFld()->SetFieldSeq(fieldSeq == nullptr ? FieldSeqStore::NotAField() : fieldSeq); - - unsigned flags = 0; - - if (user->OperIs(GT_ASG) && (user->AsOp()->gtGetOp1() == indir)) - { - flags |= GTF_VAR_DEF | GTF_DONT_CSE; - - if (genTypeSize(indir->TypeGet()) < m_compiler->lvaLclExactSize(val.LclNum())) - { - flags |= GTF_VAR_USEASG; - } - } - - indir->gtFlags = flags; - - // Promoted struct vars aren't currently handled here so the created LCL_FLD can't be - // later transformed into a LCL_VAR and the variable cannot be enregistered. - m_compiler->lvaSetVarDoNotEnregister(val.LclNum() DEBUGARG(Compiler::DNER_LocalField)); - - INDEBUG(m_stmtModified = true;) - } - - //------------------------------------------------------------------------ // MorphStructField: Replaces a GT_FIELD based promoted/normed struct field access // (e.g. FIELD(ADDR(LCL_VAR))) with a GT_LCL_VAR that references the struct field. // @@ -18722,6 +18606,7 @@ void Compiler::fgMarkAddressExposedLocals() bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement* stmt) { + GenTree* tree = stmt->GetRootNode(); assert(tree->OperGet() == GT_ASG); @@ -18792,44 +18677,31 @@ bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement* fgRemoveStmt(block, stmt->GetNextStmt()); } - GenTree* dstNode; + GenTree* copyBlkDst = createAddressNodeForSIMDInit(originalLHS, simdSize); + if (simdStructNode->OperIsLocal()) + { + setLclRelatedToSIMDIntrinsic(simdStructNode); + } + GenTree* copyBlkAddr = copyBlkDst; + if (copyBlkAddr->gtOper == GT_LEA) + { + copyBlkAddr = copyBlkAddr->AsAddrMode()->Base(); + } + GenTreeLclVarCommon* localDst = nullptr; + if (copyBlkAddr->IsLocalAddrExpr(this, &localDst, nullptr)) + { + setLclRelatedToSIMDIntrinsic(localDst); + } - if (originalLHS->OperIs(GT_LCL_FLD)) + if (simdStructNode->TypeGet() == TYP_BYREF) { - dstNode = originalLHS; - dstNode->gtType = simdType; - dstNode->AsLclFld()->SetFieldSeq(FieldSeqStore::NotAField()); + assert(simdStructNode->OperIsLocal()); + assert(lvaIsImplicitByRefLocal(simdStructNode->AsLclVarCommon()->GetLclNum())); + simdStructNode = gtNewIndir(simdType, simdStructNode); } else { - GenTree* copyBlkDst = createAddressNodeForSIMDInit(originalLHS, simdSize); - if (simdStructNode->OperIsLocal()) - { - setLclRelatedToSIMDIntrinsic(simdStructNode); - } - GenTree* copyBlkAddr = copyBlkDst; - if (copyBlkAddr->gtOper == GT_LEA) - { - copyBlkAddr = copyBlkAddr->AsAddrMode()->Base(); - } - GenTreeLclVarCommon* localDst = nullptr; - if (copyBlkAddr->IsLocalAddrExpr(this, &localDst, nullptr)) - { - setLclRelatedToSIMDIntrinsic(localDst); - } - - if (simdStructNode->TypeGet() == TYP_BYREF) - { - assert(simdStructNode->OperIsLocal()); - assert(lvaIsImplicitByRefLocal(simdStructNode->AsLclVarCommon()->GetLclNum())); - simdStructNode = gtNewIndir(simdType, simdStructNode); - } - else - { - assert(varTypeIsSIMD(simdStructNode)); - } - - dstNode = gtNewOperNode(GT_IND, simdType, copyBlkDst); + assert(varTypeIsSIMD(simdStructNode)); } #ifdef DEBUG @@ -18842,16 +18714,13 @@ bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement* } #endif - tree = gtNewAssignNode(dstNode, simdStructNode); + GenTree* dstNode = gtNewOperNode(GT_IND, simdType, copyBlkDst); + tree = gtNewAssignNode(dstNode, simdStructNode); stmt->SetRootNode(tree); // Since we generated a new address node which didn't exist before, // we should expose this address manually here. - // TODO-ADDR: Remove this when LocalAddressVisitor transforms all - // local field access into LCL_FLDs, at that point we would be - // combining 2 existing LCL_FLDs or 2 FIELDs that do not reference - // a local and thus cannot result in a new address exposed local. LocalAddressVisitor visitor(this); visitor.VisitStmt(stmt); diff --git a/src/coreclr/src/jit/simd.cpp b/src/coreclr/src/jit/simd.cpp index bef8a36..810b57e 100644 --- a/src/coreclr/src/jit/simd.cpp +++ b/src/coreclr/src/jit/simd.cpp @@ -2112,26 +2112,6 @@ bool Compiler::areFieldsContiguous(GenTree* first, GenTree* second) return false; } -//---------------------------------------------------------------------- -// areLocalFieldsContiguous: Check whether two local field are contiguous -// -// Arguments: -// first - the first local field -// second - the second local field -// -// Return Value: -// If the first field is located before second field, and they are located contiguously, -// then return true. Otherwise, return false. -// -bool Compiler::areLocalFieldsContiguous(GenTreeLclFld* first, GenTreeLclFld* second) -{ - assert(first->TypeIs(TYP_FLOAT)); - assert(second->TypeIs(TYP_FLOAT)); - - return (first->TypeGet() == second->TypeGet()) && - (first->GetLclOffs() + genTypeSize(first->TypeGet()) == second->GetLclOffs()); -} - //------------------------------------------------------------------------------- // Check whether two array element nodes are located contiguously or not. // Arguments: @@ -2197,10 +2177,6 @@ bool Compiler::areArgumentsContiguous(GenTree* op1, GenTree* op2) { return areFieldsContiguous(op1, op2); } - else if (op1->OperIs(GT_LCL_FLD) && op2->OperIs(GT_LCL_FLD)) - { - return areLocalFieldsContiguous(op1->AsLclFld(), op2->AsLclFld()); - } return false; } -- 2.7.4