From aeb7b9121db38979b23949cf584f39cbb7637133 Mon Sep 17 00:00:00 2001 From: SingleAccretion <62474226+SingleAccretion@users.noreply.github.com> Date: Sun, 6 Feb 2022 17:48:36 +0300 Subject: [PATCH] Add support for `TYP_BYREF` `LCL_FLD`s to VN (#64501) * Do not add a zero-offset FldSeq to LCL_FLD directly "fgAddFieldSeqForZeroOffset"'s contract is that it is passed an *address*. If that address is a LCL_FLD, we must use the "zero-offset sequence map", not the sequence of LCL_FLD itself, as that represents LCL_FLD's own value, not the value it produces (LCL_FLD == IND(LCL_FLD_ADDR), and LCL_FLD's sequence is the one attached to LCL_FLD_ADDR, not IND). * Fix the same issue in "ChangeOper" * Read zero-offset FldSeqs on LclFld in VN --- src/coreclr/jit/compiler.hpp | 59 +++++++++++++----------------------- src/coreclr/jit/morph.cpp | 29 ++++-------------- src/coreclr/jit/valuenum.cpp | 11 +++++++ 3 files changed, 38 insertions(+), 61 deletions(-) diff --git a/src/coreclr/jit/compiler.hpp b/src/coreclr/jit/compiler.hpp index f03ecd43f77025..5fef8c6ffe0f32 100644 --- a/src/coreclr/jit/compiler.hpp +++ b/src/coreclr/jit/compiler.hpp @@ -1321,24 +1321,33 @@ inline void GenTree::SetOper(genTreeOps oper, ValueNumberUpdate vnUpdate) SetVtableForOper(oper); #endif // DEBUGGABLE_GENTREE - if (oper == GT_CNS_INT) + if (vnUpdate == CLEAR_VN) { - AsIntCon()->gtFieldSeq = nullptr; + // Clear the ValueNum field as well. + gtVNPair.SetBoth(ValueNumStore::NoVN); } -#if defined(TARGET_ARM) - if (oper == GT_MUL_LONG) + // Do "oper"-specific initializations. TODO-Cleanup: these are too ad-hoc to be reliable. + // The bashing code should decide itself what to initialize and what to leave as it was. + switch (oper) { - // We sometimes bash GT_MUL to GT_MUL_LONG, which converts it from GenTreeOp to GenTreeMultiRegOp. - AsMultiRegOp()->gtOtherReg = REG_NA; - AsMultiRegOp()->ClearOtherRegFlags(); - } + case GT_CNS_INT: + AsIntCon()->gtFieldSeq = FieldSeqStore::NotAField(); + break; +#if defined(TARGET_ARM) + case GT_MUL_LONG: + // We sometimes bash GT_MUL to GT_MUL_LONG, which converts it from GenTreeOp to GenTreeMultiRegOp. + AsMultiRegOp()->gtOtherReg = REG_NA; + AsMultiRegOp()->ClearOtherRegFlags(); + break; #endif + case GT_LCL_FLD: + AsLclFld()->SetLclOffs(0); + AsLclFld()->SetFieldSeq(FieldSeqStore::NotAField()); + break; - if (vnUpdate == CLEAR_VN) - { - // Clear the ValueNum field as well. - gtVNPair.SetBoth(ValueNumStore::NoVN); + default: + break; } } @@ -1418,32 +1427,6 @@ inline void GenTree::ChangeOper(genTreeOps oper, ValueNumberUpdate vnUpdate) } SetOper(oper, vnUpdate); gtFlags &= mask; - - // Do "oper"-specific initializations... - switch (oper) - { - case GT_LCL_FLD: - { - // The original GT_LCL_VAR might be annotated with a zeroOffset field. - FieldSeqNode* zeroFieldSeq = nullptr; - Compiler* compiler = JitTls::GetCompiler(); - bool isZeroOffset = compiler->GetZeroOffsetFieldMap()->Lookup(this, &zeroFieldSeq); - - AsLclFld()->SetLclOffs(0); - AsLclFld()->SetFieldSeq(FieldSeqStore::NotAField()); - - if (zeroFieldSeq != nullptr) - { - // Set the zeroFieldSeq in the GT_LCL_FLD node - AsLclFld()->SetFieldSeq(zeroFieldSeq); - // and remove the annotation from the ZeroOffsetFieldMap - compiler->GetZeroOffsetFieldMap()->Remove(this); - } - break; - } - default: - break; - } } inline void GenTree::ChangeOperUnchecked(genTreeOps oper) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b3e9e8fa82977f..27300cd32d63b7 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -12591,28 +12591,20 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) lclFld->SetLclOffs(lclFld->GetLclOffs() + static_cast(ival1)); lclFld->SetFieldSeq(GetFieldSeqStore()->Append(lclFld->GetFieldSeq(), fieldSeq)); } - else // we have a GT_LCL_VAR + else // We have a GT_LCL_VAR. { assert(temp->OperGet() == GT_LCL_VAR); - temp->ChangeOper(GT_LCL_FLD); // Note that this typically makes the gtFieldSeq "NotAField", - // unless there is a zero filed offset associated with 'temp'. + temp->ChangeOper(GT_LCL_FLD); // Note that this makes the gtFieldSeq "NotAField". lclFld = temp->AsLclFld(); lclFld->SetLclOffs(static_cast(ival1)); - if (lclFld->GetFieldSeq() == FieldSeqStore::NotAField()) + if (fieldSeq != nullptr) { - if (fieldSeq != nullptr) - { - // If it does represent a field, note that. - lclFld->SetFieldSeq(fieldSeq); - } - } - else - { - // Append 'fieldSeq' to the existing one - lclFld->SetFieldSeq(GetFieldSeqStore()->Append(lclFld->GetFieldSeq(), fieldSeq)); + // If it does represent a field, note that. + lclFld->SetFieldSeq(fieldSeq); } } + temp->gtType = tree->gtType; foldAndReturnTemp = true; } @@ -17806,15 +17798,6 @@ void Compiler::fgAddFieldSeqForZeroOffset(GenTree* addr, FieldSeqNode* fieldSeqZ fieldSeqRecorded = true; break; - case GT_LCL_FLD: - { - GenTreeLclFld* lclFld = addr->AsLclFld(); - fieldSeqUpdate = GetFieldSeqStore()->Append(lclFld->GetFieldSeq(), fieldSeqZero); - lclFld->SetFieldSeq(fieldSeqUpdate); - fieldSeqRecorded = true; - break; - } - case GT_ADDR: if (addr->AsOp()->gtOp1->OperGet() == GT_LCL_FLD) { diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index 4be6e1a93a4cdb..6df1bd9c30bfa8 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8449,6 +8449,17 @@ void Compiler::fgValueNumberTree(GenTree* tree) { ValueNumPair lclVNPair = varDsc->GetPerSsaData(ssaNum)->m_vnPair; tree->gtVNPair = vnStore->VNPairApplySelectors(lclVNPair, lclFld->GetFieldSeq(), indType); + + // If we have byref field, we may have a zero-offset sequence to add. + FieldSeqNode* zeroOffsetFldSeq = nullptr; + if ((typ == TYP_BYREF) && GetZeroOffsetFieldMap()->Lookup(lclFld, &zeroOffsetFldSeq)) + { + ValueNum addrExtended = vnStore->ExtendPtrVN(lclFld, zeroOffsetFldSeq); + if (addrExtended != ValueNumStore::NoVN) + { + lclFld->gtVNPair.SetBoth(addrExtended); + } + } } } else