From 4974a676f0a7c4a21732aa6e27ef0baafd424df6 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Mon, 1 Nov 2021 02:20:42 +0300 Subject: [PATCH] Morph ASG(ONE_FLD_STRUCT, CALL) using BITCAST This changes the representation we have for the special case of assignments from calls that return a struct with one field to a promoted local of that struct type, making it more normal and simplifying the model for SSA and value numbering. In the past, the optimization phases treated this case specially and "skipped" the outer struct local in relevant cases. Instead, I propose a new form for this assignment: ASG(FLD, BITCAST(CALL)), that gets rid of this special-casing, and introduces the useful invariant that if a node has an SSA name, then it always represents a def or use of the corresponding SSA local. After optimizations have done their work, lowering removes the new bitcasts as they do not have meaningful value in the backend. There are some downsides to this representation. For example, if we decide to stop retyping helper calls in the importer, VN will start giving BITCAST VNs to locals assigned values produced by important helpers such as TypeHandleToRuntimeTypeHandle. It makes the IR a bit larger, and, perhaps more importantly, more sparse. I can measure a 0.003% increase in memory consumption when CG-ing x64 CoreLib with this change. There are some TP benefits from the reduced checks too, they may offset this regression. It seems to me that the benefits outweigh the drawbacks in this case. --- src/coreclr/jit/copyprop.cpp | 6 ----- src/coreclr/jit/earlyprop.cpp | 3 +-- src/coreclr/jit/gentree.cpp | 24 ++++++++++++++++++ src/coreclr/jit/gentree.h | 5 +--- src/coreclr/jit/lower.cpp | 45 ++++++++++++++++++++++++++++++++++ src/coreclr/jit/lower.h | 1 + src/coreclr/jit/lsrabuild.cpp | 4 +-- src/coreclr/jit/morphblock.cpp | 25 ++++++++++++++++--- src/coreclr/jit/optimizer.cpp | 6 ++--- src/coreclr/jit/rangecheck.cpp | 16 ++---------- src/coreclr/jit/ssabuilder.cpp | 7 ------ src/coreclr/jit/valuenum.cpp | 20 +-------------- 12 files changed, 102 insertions(+), 60 deletions(-) diff --git a/src/coreclr/jit/copyprop.cpp b/src/coreclr/jit/copyprop.cpp index 778dd7d404f6e..fef2e363cbb81 100644 --- a/src/coreclr/jit/copyprop.cpp +++ b/src/coreclr/jit/copyprop.cpp @@ -257,7 +257,6 @@ void Compiler::optCopyProp(Statement* stmt, GenTreeLclVarCommon* tree, unsigned // // Returns: // - lclNum if the local is participating in SSA; -// - fieldLclNum if the parent local can be replaced by its only field; // - BAD_VAR_NUM otherwise. // unsigned Compiler::optIsSsaLocal(GenTreeLclVarCommon* lclNode) @@ -265,11 +264,6 @@ unsigned Compiler::optIsSsaLocal(GenTreeLclVarCommon* lclNode) unsigned lclNum = lclNode->GetLclNum(); LclVarDsc* varDsc = lvaGetDesc(lclNum); - if (!lvaInSsa(lclNum) && varDsc->CanBeReplacedWithItsField(this)) - { - lclNum = varDsc->lvFieldLclStart; - } - if (!lvaInSsa(lclNum)) { return BAD_VAR_NUM; diff --git a/src/coreclr/jit/earlyprop.cpp b/src/coreclr/jit/earlyprop.cpp index 3adaf9e5fd0f2..3a6b744b7ee3b 100644 --- a/src/coreclr/jit/earlyprop.cpp +++ b/src/coreclr/jit/earlyprop.cpp @@ -374,8 +374,7 @@ GenTree* Compiler::optPropGetValueRec(unsigned lclNum, unsigned ssaNum, optPropK GenTree* treeRhs = ssaDefAsg->gtGetOp2(); - if (treeRhs->OperIsScalarLocal() && lvaInSsa(treeRhs->AsLclVarCommon()->GetLclNum()) && - treeRhs->AsLclVarCommon()->HasSsaName()) + if (treeRhs->OperIsScalarLocal() && treeRhs->AsLclVarCommon()->HasSsaName()) { // Recursively track the Rhs unsigned rhsLclNum = treeRhs->AsLclVarCommon()->GetLclNum(); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b15378b8e9b97..4d27b8d4becc0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -23307,6 +23307,30 @@ unsigned GenTreeHWIntrinsic::GetResultOpNumForFMA(GenTree* use, GenTree* op1, Ge } #endif // TARGET_XARCH && FEATURE_HW_INTRINSICS +//------------------------------------------------------------------------ +// HasSsaName: Does this local node have an SSA name? +// +// Return Value: +// Whether this node's SSA name is not RESERVED_SSA_NUM. +// +// Notes: +// If the node has an SSA name, it will always represent a local that +// participates in SSA. Thus, "HasSsaName" implies "lvaIsInSsa". The +// opposite is not true - a node may be in an unreachable block not +// visited by the renamer. +// +bool GenTreeLclVarCommon::HasSsaName() const +{ + bool hasSsaName = GetSsaNum() != SsaConfig::RESERVED_SSA_NUM; + + if (hasSsaName) + { + assert(JitTls::GetCompiler()->lvaInSsa(GetLclNum())); + } + + return hasSsaName; +} + unsigned GenTreeLclFld::GetSize() const { return TypeIs(TYP_STRUCT) ? GetLayout()->GetSize() : genTypeSize(TypeGet()); diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index f81aaa779b819..4929503e72226 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -3550,10 +3550,7 @@ struct GenTreeLclVarCommon : public GenTreeUnOp _gtSsaNum = ssaNum; } - bool HasSsaName() - { - return (GetSsaNum() != SsaConfig::RESERVED_SSA_NUM); - } + bool HasSsaName() const; #if DEBUGGABLE_GENTREE GenTreeLclVarCommon() : GenTreeUnOp() diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 84bb325bb1e83..7bedd6eee41d1 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -246,6 +246,9 @@ GenTree* Lowering::LowerNode(GenTree* node) LowerRet(node->AsUnOp()); break; + case GT_BITCAST: + return LowerBitcast(node->AsUnOp()); + case GT_RETURNTRAP: ContainCheckReturnTrap(node->AsOp()); break; @@ -3360,6 +3363,44 @@ void Lowering::LowerRet(GenTreeUnOp* ret) ContainCheckRet(ret); } +//------------------------------------------------------------------------ +// LowerBitcast: Lower a GT_BITCAST node. +// +// Will remove bitcasts that do not move between different register files. +// Such bitcasts are generated in block morphing for assignments of struct +// locals that "can be replaced with their one field" to calls. This makes +// the frontend (SSA) simpler, but here, in the backend, they end up as +// unnecessary no-op BITCAST(type -> type) nodes, so we remove them. +// +// Arguments: +// bitcast - the bitcast node to lower +// +// Return Value: +// The next node to lower. +// +GenTree* Lowering::LowerBitcast(GenTreeUnOp* bitcast) +{ + GenTree* nextNode = bitcast->gtNext; + GenTree* src = bitcast->gtGetOp1(); + + if (bitcast->TypeGet() == src->TypeGet()) + { + LIR::Use bitcastUse; + if (BlockRange().TryGetUse(bitcast, &bitcastUse)) + { + bitcastUse.ReplaceWith(src); + } + else + { + src->SetUnusedValue(); + } + + BlockRange().Remove(bitcast); + } + + return nextNode; +} + //---------------------------------------------------------------------------------------------- // LowerStoreLocCommon: platform idependent part of local var or field store lowering. // @@ -3814,6 +3855,10 @@ void Lowering::LowerCallStruct(GenTreeCall* call) GenTree* user = callUse.User(); switch (user->OperGet()) { + case GT_BITCAST: + assert(genTypeSize(user) == genTypeSize(call)); + break; + case GT_RETURN: case GT_STORE_LCL_VAR: case GT_STORE_BLK: diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 46e1accf726cb..f3b4ad8d7e450 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -136,6 +136,7 @@ class Lowering final : public Phase GenTreeCC* LowerNodeCC(GenTree* node, GenCondition condition); void LowerJmpMethod(GenTree* jmp); void LowerRet(GenTreeUnOp* ret); + GenTree* LowerBitcast(GenTreeUnOp* node); void LowerStoreLocCommon(GenTreeLclVarCommon* lclVar); void LowerRetStruct(GenTreeUnOp* ret); void LowerRetSingleRegStructLclVar(GenTreeUnOp* ret); diff --git a/src/coreclr/jit/lsrabuild.cpp b/src/coreclr/jit/lsrabuild.cpp index bb02dcc9de9d3..720866c4ad6aa 100644 --- a/src/coreclr/jit/lsrabuild.cpp +++ b/src/coreclr/jit/lsrabuild.cpp @@ -3523,11 +3523,11 @@ int LinearScan::BuildStoreLoc(GenTreeLclVarCommon* storeLoc) else if (op1->isContained() && op1->OperIs(GT_BITCAST)) { GenTree* bitCastSrc = op1->gtGetOp1(); - RegisterType registerType = bitCastSrc->TypeGet(); + RegisterType registerType = regType(getDefType(bitCastSrc)); singleUseRef = BuildUse(bitCastSrc, allRegs(registerType)); Interval* srcInterval = singleUseRef->getInterval(); - assert(srcInterval->registerType == registerType); + assert(srcInterval->registerType == getDefType(bitCastSrc)); srcCount = 1; } #ifndef TARGET_64BIT diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 0346eeb2456ec..9c881377a323f 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -54,6 +54,7 @@ class MorphInitBlockHelper OneAsgBlock, StructBlock, SkipCallSrc, + BitcastCallSrc, SkipMultiRegIntrinsicSrc, Nop }; @@ -806,15 +807,33 @@ void MorphCopyBlockHelper::TrySpecialCases() m_asg->gtOp1 = lclVar; } } + if (m_dst->OperIs(GT_LCL_VAR)) { LclVarDsc* varDsc = m_comp->lvaGetDesc(m_dst->AsLclVar()); if (varTypeIsStruct(varDsc) && varDsc->CanBeReplacedWithItsField(m_comp)) { + assert(m_comp->fgGlobalMorph); + + JITDUMP("Morphing a single reg call return into BITCAST assigned to the promoted field\n"); + m_transformationDecision = BlockTransformation::BitcastCallSrc; + + unsigned fieldNum = varDsc->lvFieldLclStart; + LclVarDsc* fieldDsc = m_comp->lvaGetDesc(fieldNum); + var_types fieldType = fieldDsc->TypeGet(); + assert(!varTypeIsStruct(fieldType)); + m_dst->gtFlags |= GTF_DONT_CSE; - JITDUMP("Not morphing a single reg call return\n"); - m_transformationDecision = BlockTransformation::SkipCallSrc; - m_result = m_asg; + m_dst->ChangeType(fieldType); + m_dst->AsLclVar()->SetLclNum(fieldNum); + + assert(fieldDsc->lvNormalizeOnLoad() || !varTypeIsSmall(fieldDsc)); + GenTree* bitcast = m_comp->gtNewOperNode(GT_BITCAST, genActualType(fieldType), m_src); + m_src = bitcast; + m_asg->gtOp2 = bitcast; + + m_asg->ChangeType(fieldType); + m_result = m_asg; } } } diff --git a/src/coreclr/jit/optimizer.cpp b/src/coreclr/jit/optimizer.cpp index 55cebdb5597bc..eae3794031ee4 100644 --- a/src/coreclr/jit/optimizer.cpp +++ b/src/coreclr/jit/optimizer.cpp @@ -7264,7 +7264,7 @@ void Compiler::optHoistLoopBlocks(unsigned loopNum, ArrayStack* blo // To be invariant a LclVar node must not be the LHS of an assignment ... bool isInvariant = !user->OperIs(GT_ASG) || (user->AsOp()->gtGetOp1() != tree); // and the variable must be in SSA ... - isInvariant = isInvariant && m_compiler->lvaInSsa(lclNum) && lclVar->HasSsaName(); + isInvariant = isInvariant && lclVar->HasSsaName(); // and the SSA definition must be outside the loop we're hoisting from ... isInvariant = isInvariant && !m_compiler->optLoopTable[m_loopNum].lpContains( @@ -8455,7 +8455,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) { // If it's a local byref for which we recorded a value number, use that... GenTreeLclVar* argLcl = arg->AsLclVar(); - if (lvaInSsa(argLcl->GetLclNum()) && argLcl->HasSsaName()) + if (argLcl->HasSsaName()) { ValueNum argVN = lvaTable[argLcl->GetLclNum()].GetPerSsaData(argLcl->GetSsaNum())->m_vnPair.GetLiberal(); @@ -8533,7 +8533,7 @@ bool Compiler::optComputeLoopSideEffectsOfBlock(BasicBlock* blk) if (rhsVN != ValueNumStore::NoVN) { rhsVN = vnStore->VNNormalValue(rhsVN); - if (lvaInSsa(lhsLcl->GetLclNum()) && lhsLcl->HasSsaName()) + if (lhsLcl->HasSsaName()) { lvaTable[lhsLcl->GetLclNum()] .GetPerSsaData(lhsLcl->GetSsaNum()) diff --git a/src/coreclr/jit/rangecheck.cpp b/src/coreclr/jit/rangecheck.cpp index b94f124d00328..3cf6e4c97f74f 100644 --- a/src/coreclr/jit/rangecheck.cpp +++ b/src/coreclr/jit/rangecheck.cpp @@ -454,11 +454,7 @@ LclSsaVarDsc* RangeCheck::GetSsaDefAsg(GenTreeLclVarCommon* lclUse) return nullptr; } - LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lclUse); - if (varDsc->CanBeReplacedWithItsField(m_pCompiler)) - { - varDsc = m_pCompiler->lvaGetDesc(varDsc->lvFieldLclStart); - } + LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lclUse); LclSsaVarDsc* ssaDef = varDsc->GetPerSsaData(ssaNum); // RangeCheck does not care about uninitialized variables. @@ -493,10 +489,6 @@ LclSsaVarDsc* RangeCheck::GetSsaDefAsg(GenTreeLclVarCommon* lclUse) UINT64 RangeCheck::HashCode(unsigned lclNum, unsigned ssaNum) { LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lclNum); - if (varDsc->CanBeReplacedWithItsField(m_pCompiler)) - { - lclNum = varDsc->lvFieldLclStart; - } assert(ssaNum != SsaConfig::RESERVED_SSA_NUM); return UINT64(lclNum) << 32 | ssaNum; } @@ -563,11 +555,7 @@ void RangeCheck::MergeEdgeAssertions(GenTreeLclVarCommon* lcl, ASSERT_VALARG_TP return; } - LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lcl); - if (varDsc->CanBeReplacedWithItsField(m_pCompiler)) - { - varDsc = m_pCompiler->lvaGetDesc(varDsc->lvFieldLclStart); - } + LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lcl); LclSsaVarDsc* ssaData = varDsc->GetPerSsaData(lcl->GetSsaNum()); ValueNum normalLclVN = m_pCompiler->vnStore->VNConservativeNormalValue(ssaData->m_vnPair); MergeEdgeAssertions(normalLclVN, assertions, pRange); diff --git a/src/coreclr/jit/ssabuilder.cpp b/src/coreclr/jit/ssabuilder.cpp index 7f62e8c38c762..da1080ee1017b 100644 --- a/src/coreclr/jit/ssabuilder.cpp +++ b/src/coreclr/jit/ssabuilder.cpp @@ -745,13 +745,6 @@ void SsaBuilder::RenameDef(GenTree* defNode, BasicBlock* block) unsigned lclNum = lclNode->GetLclNum(); LclVarDsc* varDsc = m_pCompiler->lvaGetDesc(lclNum); - if (!m_pCompiler->lvaInSsa(lclNum) && varDsc->CanBeReplacedWithItsField(m_pCompiler)) - { - lclNum = varDsc->lvFieldLclStart; - varDsc = m_pCompiler->lvaGetDesc(lclNum); - assert(isFullDef); - } - if (m_pCompiler->lvaInSsa(lclNum)) { // Promoted variables are not in SSA, only their fields are. diff --git a/src/coreclr/jit/valuenum.cpp b/src/coreclr/jit/valuenum.cpp index b203ea7b10784..dd02a74c80207 100644 --- a/src/coreclr/jit/valuenum.cpp +++ b/src/coreclr/jit/valuenum.cpp @@ -8188,24 +8188,6 @@ void Compiler::fgValueNumberBlockAssignment(GenTree* tree) fgValueNumberLocalStore(tree, lclVarTree, offset, storeSize, rhsVNPair); } - else if (lclVarTree->HasSsaName()) - { - // The local wasn't in SSA, the tree is still an SSA def. There is only one - // case when this can happen - a promoted "CanBeReplacedWithItsField" struct. - assert((lhs == lclVarTree) && rhs->IsCall() && isEntire); - assert(lhsVarDsc->CanBeReplacedWithItsField(this)); - // Give a new, unique, VN to the field. - LclVarDsc* fieldVarDsc = lvaGetDesc(lhsVarDsc->lvFieldLclStart); - LclSsaVarDsc* fieldVarSsaDsc = fieldVarDsc->GetPerSsaData(lclVarTree->GetSsaNum()); - ValueNum newUniqueVN = vnStore->VNForExpr(compCurBB, fieldVarDsc->TypeGet()); - - fieldVarSsaDsc->m_vnPair.SetBoth(newUniqueVN); - - JITDUMP("Tree [%06u] assigned VN to the only field V%02u/%u of promoted struct V%02u: new uniq ", - dspTreeID(tree), lhsVarDsc->lvFieldLclStart, lclVarTree->GetSsaNum(), lhsLclNum); - JITDUMPEXEC(vnPrint(newUniqueVN, 1)); - JITDUMP("\n"); - } else if (lhsVarDsc->IsAddressExposed()) { fgMutateAddressExposedLocal(tree DEBUGARG("INITBLK/COPYBLK - address-exposed local")); @@ -8324,7 +8306,7 @@ void Compiler::fgValueNumberTree(GenTree* tree) { unsigned lclNum = lclFld->GetLclNum(); - if (!lvaInSsa(lclFld->GetLclNum()) || !lclFld->HasSsaName()) + if (!lclFld->HasSsaName()) { lclFld->gtVNPair.SetBoth(vnStore->VNForExpr(compCurBB, lclFld->TypeGet())); }