Fix for issue 5954 - Assert 'fieldSeq != FieldSeqStore::NotAField()'

Added new method GenTree:OperIsLocalField()
Fixes DefinesLocalAddress to properly update pIsEntire when we have a LocalField node type
Fixed fgValueNumberBlockAssignment to handle a GT_COPYBLK or GT_COPYOBJ operation
  where we have an incomplete field sequence for the Dst/LHS node.
  We already were handling an incomplete field sequence for the Src/RHS node.
  Update the dump to properly print "new uniq" whenever we issue a conservative value number.

Commit migrated from https://github.com/dotnet/coreclr/commit/7611127a4f7757952ff0433ec386d8bb3d55d8f1

diff --git a/src/coreclr/src/jit/gentree.cpp b/src/coreclr/src/jit/gentree.cpp
index 947fde6..26769ef 100644 (file)
--- a/src/coreclr/src/jit/gentree.cpp
+++ b/src/coreclr/src/jit/gentree.cpp
@@ -13582,15 +13582,29 @@ bool GenTree::DefinesLocalAddr(Compiler* comp, unsigned width, GenTreeLclVarComm
             *pLclVarTree = addrArgLcl;
             if (pIsEntire != NULL)
             {
-                unsigned lclNum = addrArgLcl->GetLclNum();
-                unsigned varWidth = comp->lvaLclExactSize(lclNum);
-                if (comp->lvaTable[lclNum].lvNormalizeOnStore())
+                unsigned lclOffset = 0;
+                if (addrArg->OperIsLocalField())
                 {
-                    // It's normalize on store, so use the full storage width -- writing to low bytes won't
-                    // necessarily yield a normalized value.
-                    varWidth = genTypeStSz(var_types(comp->lvaTable[lclNum].lvType)) * sizeof(int);
+                    lclOffset = addrArg->gtLclFld.gtLclOffs;
+                }
+
+                if (lclOffset != 0)
+                {
+                    // We aren't updating the bytes at [0..lclOffset-1] so *pIsEntire should be set to false
+                    *pIsEntire = false;
+                }
+                else
+                {
+                    unsigned lclNum = addrArgLcl->GetLclNum();
+                    unsigned varWidth = comp->lvaLclExactSize(lclNum);
+                    if (comp->lvaTable[lclNum].lvNormalizeOnStore())
+                    {
+                        // It's normalize on store, so use the full storage width -- writing to low bytes won't
+                        // necessarily yield a normalized value.
+                        varWidth = genTypeStSz(var_types(comp->lvaTable[lclNum].lvType)) * sizeof(int);
+                    }
+                    *pIsEntire = (varWidth == width);
                 }
-                *pIsEntire = (varWidth == width);
             }
             return true;
         }

diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h
index dd02d91..edfcfd6 100644 (file)
--- a/src/coreclr/src/jit/gentree.h
+++ b/src/coreclr/src/jit/gentree.h
@@ -971,6 +971,19 @@ public:
     }
 
     static
+    bool            OperIsLocalField(genTreeOps gtOper)
+    {
+        return (gtOper == GT_LCL_FLD       ||
+                gtOper == GT_LCL_FLD_ADDR  ||
+                gtOper == GT_STORE_LCL_FLD   );
+    }
+
+    inline bool     OperIsLocalField() const
+    {
+        return OperIsLocalField(gtOper);
+    }
+
+    static
     bool           OperIsScalarLocal(genTreeOps gtOper)
     {
         return (gtOper == GT_LCL_VAR ||

diff --git a/src/coreclr/src/jit/liveness.cpp b/src/coreclr/src/jit/liveness.cpp
index b761bb2..f8c7227 100644 (file)
--- a/src/coreclr/src/jit/liveness.cpp
+++ b/src/coreclr/src/jit/liveness.cpp
@@ -42,7 +42,7 @@ void                 Compiler::fgMarkUseDef(GenTreeLclVarCommon *tree, GenTree *
     }
     else
     {
-        noway_assert(tree->gtOper == GT_LCL_FLD || tree->gtOper == GT_LCL_FLD_ADDR || tree->gtOper == GT_STORE_LCL_FLD);
+        noway_assert(tree->OperIsLocalField());
         lclNum = tree->gtLclFld.gtLclNum;
     }
 

diff --git a/src/coreclr/src/jit/valuenum.cpp b/src/coreclr/src/jit/valuenum.cpp
index 9165fca..5ebbc48 100644 (file)
--- a/src/coreclr/src/jit/valuenum.cpp
+++ b/src/coreclr/src/jit/valuenum.cpp
@@ -4748,19 +4748,15 @@ void Compiler::fgValueNumberBlockAssignment(GenTreePtr tree, bool evalAsgLhsInd)
                 GenTreeLclVarCommon* rhsLclVarTree = nullptr;
                 FieldSeqNode* rhsFldSeq = nullptr;
                 ValueNumPair rhsVNPair;
-#ifdef DEBUG
                 bool isNewUniq = false;
-#endif
+
                 if (srcAddr->IsLocalAddrExpr(this, &rhsLclVarTree, &rhsFldSeq))
                 {
                     unsigned    rhsLclNum = rhsLclVarTree->GetLclNum();
                     LclVarDsc*  rhsVarDsc = &lvaTable[rhsLclNum];
                     if (fgExcludeFromSsa(rhsLclNum) || rhsFldSeq == FieldSeqStore::NotAField())
                     {
-                        rhsVNPair.SetBoth(vnStore->VNForExpr(lclVarTree->TypeGet()));
-#ifdef DEBUG
                         isNewUniq = true;
-#endif
                     }
                     else
                     {
@@ -4798,7 +4794,7 @@ void Compiler::fgValueNumberBlockAssignment(GenTreePtr tree, bool evalAsgLhsInd)
                         }
                         else
                         {
-                            JITDUMP("    *** Missing field sequence info for COPYBLK\n");
+                            JITDUMP("    *** Missing field sequence info for Src/RHS of COPYBLK\n");
                             rhsVNPair.SetBoth(vnStore->VNForExpr(indType)); //  a new unique value number
                         }
                     }
@@ -4810,51 +4806,58 @@ void Compiler::fgValueNumberBlockAssignment(GenTreePtr tree, bool evalAsgLhsInd)
                     }
                     else
                     {
-                        rhsVNPair.SetBoth(vnStore->VNForExpr(lclVarTree->TypeGet()));
-#ifdef DEBUG
                         isNewUniq = true;
-#endif
                     }
                 }
                 else
                 {
-                    rhsVNPair.SetBoth(vnStore->VNForExpr(lclVarTree->TypeGet()));
-#ifdef DEBUG
-                    isNewUniq = true;
-#endif
+                   isNewUniq = true;
                 }
 
-                ValueNumPair newRhsVNPair;
-                if (lhsFldSeq != nullptr && isEntire)
+                if (lhsFldSeq == FieldSeqStore::NotAField())
+                {
+                    // We don't have proper field sequence information for the lhs
+                    //
+                    JITDUMP("    *** Missing field sequence info for Dst/LHS of COPYBLK\n");
+                    isNewUniq = true;
+                }
+                else if (lhsFldSeq != nullptr && isEntire)
                 {
                     // This can occur in for structs with one field, itself of a struct type.
                     // We won't promote these.
                     // TODO-Cleanup: decide what exactly to do about this.
                     // Always treat them as maps, making them use/def, or reconstitute the
                     // map view here?
-                    newRhsVNPair.SetBoth(vnStore->VNForExpr(TYP_STRUCT));
+                    isNewUniq = true;
                 }
-                else
+                else if (!isNewUniq)
                 {
                     ValueNumPair oldLhsVNPair = lvaTable[lhsLclNum].GetPerSsaData(lclVarTree->GetSsaNum())->m_vnPair;
-                    newRhsVNPair = vnStore->VNPairApplySelectorsAssign(oldLhsVNPair, lhsFldSeq, rhsVNPair, lclVarTree->TypeGet());
+                    rhsVNPair = vnStore->VNPairApplySelectorsAssign(oldLhsVNPair, lhsFldSeq, rhsVNPair, lclVarTree->TypeGet());
                 }
-                lvaTable[lhsLclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair = vnStore->VNPNormVal(newRhsVNPair);
+
+                if (isNewUniq)
+                {
+                    rhsVNPair.SetBoth(vnStore->VNForExpr(lclVarTree->TypeGet()));
+                }
+                
+                lvaTable[lhsLclNum].GetPerSsaData(lclDefSsaNum)->m_vnPair = vnStore->VNPNormVal(rhsVNPair);
+
 #ifdef DEBUG
                 if (verbose)
                 {
                     printf("Tree ");
                     Compiler::printTreeID(tree);
-                    printf(" assigned VN to local var V%02u/%d: ", 
-                            lhsLclNum, lclDefSsaNum);
+                    printf(" assigned VN to local var V%02u/%d: ", lhsLclNum, lclDefSsaNum);
                     if (isNewUniq)
                         printf("new uniq ");
-                    vnpPrint(newRhsVNPair, 1);
+                    vnpPrint(rhsVNPair, 1);
                     printf("\n");
                 }
 #endif // DEBUG
-            }
-        }
+
+            } 
+       }
         else
         {
             // For now, arbitrary side effect on Heap.