Revert "Start generating LCL_FLDs in LocalAddressVisitor (#737)" (#973)
authorSergey Andreenko <seandree@microsoft.com>
Tue, 17 Dec 2019 16:17:26 +0000 (08:17 -0800)
committerJan Kotas <jkotas@microsoft.com>
Tue, 17 Dec 2019 16:17:26 +0000 (08:17 -0800)
This reverts commit 4fa163852af62908948e8108a03f4044f96d2bc1.

src/coreclr/src/jit/compiler.h
src/coreclr/src/jit/gentree.cpp
src/coreclr/src/jit/gentree.h
src/coreclr/src/jit/morph.cpp
src/coreclr/src/jit/simd.cpp

index bfc7bba..869267d 100644 (file)
@@ -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);
index 8c0e992..02fd018 100644 (file)
@@ -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;
 }
index 47c8a38..c88ba80 100644 (file)
@@ -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
index c600733..75ceb05 100644 (file)
@@ -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<LocalAddressVisitor>
         }
 
         //------------------------------------------------------------------------
-        // 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);
 
index bef8a36..810b57e 100644 (file)
@@ -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;
 }