Restore "Start generating LCL_FLDs in LocalAddressVisitor #737" (#991)
authormikedn <onemihaid@hotmail.com>
Fri, 20 Dec 2019 01:26:23 +0000 (03:26 +0200)
committerSergey Andreenko <seandree@microsoft.com>
Fri, 20 Dec 2019 01:26:23 +0000 (17:26 -0800)
* Revert "Revert "Start generating LCL_FLDs in LocalAddressVisitor (#737)" (#973)"

This reverts commit 9bf91f8d178e09bb81fb1749a82d588cbe8028cf.

* Add a test that produces an unused, top-level indir

* If an indir has no user then it's not a def

* Fix test return code

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
src/coreclr/tests/src/JIT/Methodical/Boxing/misc/unusedindir.cs [new file with mode: 0644]
src/coreclr/tests/src/JIT/Methodical/Boxing/misc/unusedindir.csproj [new file with mode: 0644]

index 869267d..bfc7bba 100644 (file)
@@ -7955,6 +7955,7 @@ 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 02fd018..8c0e992 100644 (file)
@@ -18009,7 +18009,7 @@ bool FieldSeqNode::IsConstantIndexFieldSeq()
     return m_fieldHnd == FieldSeqStore::ConstantIndexPseudoField;
 }
 
-bool FieldSeqNode::IsPseudoField()
+bool FieldSeqNode::IsPseudoField() const
 {
     return m_fieldHnd == FieldSeqStore::FirstElemPseudoField || m_fieldHnd == FieldSeqStore::ConstantIndexPseudoField;
 }
index c88ba80..47c8a38 100644 (file)
@@ -235,7 +235,23 @@ struct FieldSeqNode
     bool IsConstantIndexFieldSeq();
 
     // returns true when this is the the pseudo #FirstElem field sequence or the pseudo #ConstantIndex field sequence
-    bool IsPseudoField();
+    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;
+    }
 
     // Make sure this provides methods that allow it to be used as a KeyFuncs type in SimplerHash.
     static int GetHashCode(FieldSeqNode fsn)
@@ -3244,6 +3260,13 @@ 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()
     {
@@ -5038,6 +5061,18 @@ 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;
@@ -5089,18 +5124,6 @@ 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 75ceb05..404cb60 100644 (file)
@@ -5448,18 +5448,23 @@ GenTree* Compiler::fgMorphArrayIndex(GenTree* tree)
         GenTree* arrRef2 = nullptr; // The second copy will be used in array address expression
         GenTree* index2  = nullptr;
 
-        // 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.)
+        // 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.
         //
+        // 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->OperGet() == GT_FIELD))
+            gtComplexityExceeds(&arrRef, MAX_ARR_COMPLEXITY) || arrRef->OperIs(GT_FIELD, GT_LCL_FLD))
         {
             unsigned arrRefTmpNum = lvaGrabTemp(true DEBUGARG("arr expr"));
             arrRefDefn            = gtNewTempAssign(arrRefTmpNum, arrRef);
@@ -5472,16 +5477,10 @@ 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) ||
-            (arrRef->OperGet() == GT_FIELD))
+            index->OperIs(GT_FIELD, GT_LCL_FLD))
         {
-            unsigned indexTmpNum = lvaGrabTemp(true DEBUGARG("arr expr"));
+            unsigned indexTmpNum = lvaGrabTemp(true DEBUGARG("index expr"));
             indexDefn            = gtNewTempAssign(indexTmpNum, index);
             index                = gtNewLclvNode(indexTmpNum, index->TypeGet());
             index2               = gtNewLclvNode(indexTmpNum, index->TypeGet());
@@ -5759,7 +5758,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
@@ -8551,9 +8550,14 @@ 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(),
@@ -8567,8 +8571,8 @@ GenTree* Compiler::fgMorphLeaf(GenTree* tree)
                 return newTree;
             }
         }
-    }
 #endif // _TARGET_X86_
+    }
     else if (tree->gtOper == GT_FTN_ADDR)
     {
         CORINFO_CONST_LOOKUP addrInfo;
@@ -17655,7 +17659,7 @@ class LocalAddressVisitor final : public GenTreeVisitor<LocalAddressVisitor>
         }
 
         //------------------------------------------------------------------------
-        // Location: Produce an address value from a GT_LCL_VAR_ADDR node.
+        // Address: Produce an address value from a GT_LCL_VAR_ADDR node.
         //
         // Arguments:
         //    lclVar - a GT_LCL_VAR_ADDR node that defines the address
@@ -18247,6 +18251,14 @@ 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();)
@@ -18371,6 +18383,110 @@ 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 != nullptr) && 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.
     //
@@ -18606,7 +18722,6 @@ void Compiler::fgMarkAddressExposedLocals()
 
 bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement* stmt)
 {
-
     GenTree* tree = stmt->GetRootNode();
     assert(tree->OperGet() == GT_ASG);
 
@@ -18677,31 +18792,44 @@ bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement*
         fgRemoveStmt(block, stmt->GetNextStmt());
     }
 
-    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);
-    }
+    GenTree* dstNode;
 
-    if (simdStructNode->TypeGet() == TYP_BYREF)
+    if (originalLHS->OperIs(GT_LCL_FLD))
     {
-        assert(simdStructNode->OperIsLocal());
-        assert(lvaIsImplicitByRefLocal(simdStructNode->AsLclVarCommon()->GetLclNum()));
-        simdStructNode = gtNewIndir(simdType, simdStructNode);
+        dstNode         = originalLHS;
+        dstNode->gtType = simdType;
+        dstNode->AsLclFld()->SetFieldSeq(FieldSeqStore::NotAField());
     }
     else
     {
-        assert(varTypeIsSIMD(simdStructNode));
+        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);
     }
 
 #ifdef DEBUG
@@ -18714,13 +18842,16 @@ bool Compiler::fgMorphCombineSIMDFieldAssignments(BasicBlock* block, Statement*
     }
 #endif
 
-    GenTree* dstNode = gtNewOperNode(GT_IND, simdType, copyBlkDst);
-    tree             = gtNewAssignNode(dstNode, simdStructNode);
+    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 810b57e..bef8a36 100644 (file)
@@ -2112,6 +2112,26 @@ 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:
@@ -2177,6 +2197,10 @@ 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;
 }
 
diff --git a/src/coreclr/tests/src/JIT/Methodical/Boxing/misc/unusedindir.cs b/src/coreclr/tests/src/JIT/Methodical/Boxing/misc/unusedindir.cs
new file mode 100644 (file)
index 0000000..970ca66
--- /dev/null
@@ -0,0 +1,39 @@
+// Licensed to the .NET Foundation under one or more agreements.
+// The .NET Foundation licenses this file to you under the MIT license.
+// See the LICENSE file in the project root for more information.
+
+using System;
+using System.Runtime.CompilerServices;
+
+class Program
+{
+    struct NotPromoted
+    {
+        public int a, b, c, d, e, f;
+    }
+
+    class TypeWithStruct
+    {
+        public NotPromoted small;
+
+        public TypeWithStruct() => small.c = 100;
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static void Escape(bool b)
+    {
+    }
+
+    [MethodImpl(MethodImplOptions.NoInlining)]
+    static NotPromoted Test(TypeWithStruct obj)
+    {
+        NotPromoted t = obj.small;
+        // Try to create an OBJ(ADDR(LCL_VAR)) tree that gtTryRemoveBoxUpstreamEffects
+        // does not remove due to a spurios exception side effect nor it parents it to
+        // a COMMA like many other unused trees are.
+        Escape(Unsafe.As<NotPromoted, NotPromoted>(ref t).GetType() == typeof(NotPromoted));
+        return t;
+    }
+
+    static int Main() => Test(new TypeWithStruct()).c;
+}
diff --git a/src/coreclr/tests/src/JIT/Methodical/Boxing/misc/unusedindir.csproj b/src/coreclr/tests/src/JIT/Methodical/Boxing/misc/unusedindir.csproj
new file mode 100644 (file)
index 0000000..1554c20
--- /dev/null
@@ -0,0 +1,12 @@
+<Project Sdk="Microsoft.NET.Sdk">
+  <PropertyGroup>
+    <OutputType>Exe</OutputType>
+  </PropertyGroup>
+  <PropertyGroup>
+    <DebugType>None</DebugType>
+    <Optimize>True</Optimize>
+  </PropertyGroup>
+  <ItemGroup>
+    <Compile Include="unusedindir.cs" />
+  </ItemGroup>
+</Project>