From e7e4aacbe33fd2ab84d121b7a101e1d02ca264e2 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 6 Jun 2021 03:55:19 +0300 Subject: [PATCH 1/4] Strongly type StoreInd lowering --- src/coreclr/jit/lower.cpp | 23 +++++++++++------------ src/coreclr/jit/lower.h | 6 +++--- src/coreclr/jit/lowerarmarch.cpp | 4 ++-- src/coreclr/jit/lowerxarch.cpp | 4 ++-- 4 files changed, 18 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/lower.cpp b/src/coreclr/jit/lower.cpp index 242c3717904ae1..a73d7c47366db8 100644 --- a/src/coreclr/jit/lower.cpp +++ b/src/coreclr/jit/lower.cpp @@ -116,7 +116,7 @@ GenTree* Lowering::LowerNode(GenTree* node) break; case GT_STOREIND: - LowerStoreIndirCommon(node->AsIndir()); + LowerStoreIndirCommon(node->AsStoreInd()); break; case GT_ADD: @@ -3548,7 +3548,7 @@ void Lowering::LowerStoreSingleRegCallStruct(GenTreeBlk* store) { store->ChangeType(regType); store->SetOper(GT_STOREIND); - LowerStoreIndirCommon(store); + LowerStoreIndirCommon(store->AsStoreInd()); return; } else @@ -4088,7 +4088,7 @@ void Lowering::InsertPInvokeMethodProlog() // The init routine sets InlinedCallFrame's m_pNext, so we just set the thead's top-of-stack GenTree* frameUpd = CreateFrameLinkUpdate(PushFrame); firstBlockRange.InsertBefore(insertionPoint, LIR::SeqTree(comp, frameUpd)); - ContainCheckStoreIndir(frameUpd->AsIndir()); + ContainCheckStoreIndir(frameUpd->AsStoreInd()); DISPTREERANGE(firstBlockRange, frameUpd); } #endif // TARGET_64BIT @@ -4151,7 +4151,7 @@ void Lowering::InsertPInvokeMethodEpilog(BasicBlock* returnBB DEBUGARG(GenTree* { GenTree* frameUpd = CreateFrameLinkUpdate(PopFrame); returnBlockRange.InsertBefore(insertionPoint, LIR::SeqTree(comp, frameUpd)); - ContainCheckStoreIndir(frameUpd->AsIndir()); + ContainCheckStoreIndir(frameUpd->AsStoreInd()); } } @@ -4313,7 +4313,7 @@ void Lowering::InsertPInvokeCallProlog(GenTreeCall* call) // Stubs do this once per stub, not once per call. GenTree* frameUpd = CreateFrameLinkUpdate(PushFrame); BlockRange().InsertBefore(insertBefore, LIR::SeqTree(comp, frameUpd)); - ContainCheckStoreIndir(frameUpd->AsIndir()); + ContainCheckStoreIndir(frameUpd->AsStoreInd()); } #endif // TARGET_64BIT @@ -4323,7 +4323,7 @@ void Lowering::InsertPInvokeCallProlog(GenTreeCall* call) // [tcb + offsetOfGcState] = 0 GenTree* storeGCState = SetGCState(0); BlockRange().InsertBefore(insertBefore, LIR::SeqTree(comp, storeGCState)); - ContainCheckStoreIndir(storeGCState->AsIndir()); + ContainCheckStoreIndir(storeGCState->AsStoreInd()); // Indicate that codegen has switched this thread to preemptive GC. // This tree node doesn't generate any code, but impacts LSRA and gc reporting. @@ -4369,7 +4369,7 @@ void Lowering::InsertPInvokeCallEpilog(GenTreeCall* call) GenTree* tree = SetGCState(1); BlockRange().InsertBefore(insertionPoint, LIR::SeqTree(comp, tree)); - ContainCheckStoreIndir(tree->AsIndir()); + ContainCheckStoreIndir(tree->AsStoreInd()); tree = CreateReturnTrapSeq(); BlockRange().InsertBefore(insertionPoint, LIR::SeqTree(comp, tree)); @@ -4384,7 +4384,7 @@ void Lowering::InsertPInvokeCallEpilog(GenTreeCall* call) { tree = CreateFrameLinkUpdate(PopFrame); BlockRange().InsertBefore(insertionPoint, LIR::SeqTree(comp, tree)); - ContainCheckStoreIndir(tree->AsIndir()); + ContainCheckStoreIndir(tree->AsStoreInd()); } #else const CORINFO_EE_INFO::InlinedCallFrameInfo& callFrameInfo = comp->eeGetEEInfo()->inlinedCallFrameInfo; @@ -6376,7 +6376,7 @@ void Lowering::ContainCheckNode(GenTree* node) ContainCheckReturnTrap(node->AsOp()); break; case GT_STOREIND: - ContainCheckStoreIndir(node->AsIndir()); + ContainCheckStoreIndir(node->AsStoreInd()); break; case GT_IND: ContainCheckIndir(node->AsIndir()); @@ -6557,9 +6557,8 @@ void Lowering::ContainCheckBitCast(GenTree* node) // Arguments: // ind - the store indirection node we are lowering. // -void Lowering::LowerStoreIndirCommon(GenTreeIndir* ind) +void Lowering::LowerStoreIndirCommon(GenTreeStoreInd* ind) { - assert(ind->OperIs(GT_STOREIND)); assert(ind->TypeGet() != TYP_STRUCT); TryCreateAddrMode(ind->Addr(), true); if (!comp->codeGen->gcInfo.gcIsWriteBarrierStoreIndNode(ind)) @@ -6759,6 +6758,6 @@ bool Lowering::TryTransformStoreObjAsStoreInd(GenTreeBlk* blkNode) { assert(src->TypeIs(regType) || src->IsCnsIntOrI() || src->IsCall()); } - LowerStoreIndirCommon(blkNode); + LowerStoreIndirCommon(blkNode->AsStoreInd()); return true; } diff --git a/src/coreclr/jit/lower.h b/src/coreclr/jit/lower.h index 7f7c9d3760aa14..a0d897e74da16b 100644 --- a/src/coreclr/jit/lower.h +++ b/src/coreclr/jit/lower.h @@ -89,7 +89,7 @@ class Lowering final : public Phase void ContainCheckBitCast(GenTree* node); void ContainCheckCallOperands(GenTreeCall* call); void ContainCheckIndir(GenTreeIndir* indirNode); - void ContainCheckStoreIndir(GenTreeIndir* indirNode); + void ContainCheckStoreIndir(GenTreeStoreInd* indirNode); void ContainCheckMul(GenTreeOp* node); void ContainCheckShiftRotate(GenTreeOp* node); void ContainCheckStoreLoc(GenTreeLclVarCommon* storeLoc) const; @@ -292,9 +292,9 @@ class Lowering final : public Phase #endif // defined(TARGET_XARCH) // Per tree node member functions - void LowerStoreIndirCommon(GenTreeIndir* ind); + void LowerStoreIndirCommon(GenTreeStoreInd* ind); void LowerIndir(GenTreeIndir* ind); - void LowerStoreIndir(GenTreeIndir* node); + void LowerStoreIndir(GenTreeStoreInd* node); GenTree* LowerAdd(GenTreeOp* node); bool LowerUnsignedDivOrMod(GenTreeOp* divMod); GenTree* LowerConstIntDivOrMod(GenTree* node); diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 6774a4ef1766b4..5571f89a7c9631 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -218,7 +218,7 @@ void Lowering::LowerStoreLoc(GenTreeLclVarCommon* storeLoc) // Return Value: // None. // -void Lowering::LowerStoreIndir(GenTreeIndir* node) +void Lowering::LowerStoreIndir(GenTreeStoreInd* node) { ContainCheckStoreIndir(node); } @@ -1376,7 +1376,7 @@ void Lowering::ContainCheckCallOperands(GenTreeCall* call) // Arguments: // node - pointer to the node // -void Lowering::ContainCheckStoreIndir(GenTreeIndir* node) +void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) { #ifdef TARGET_ARM64 GenTree* src = node->AsOp()->gtOp2; diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 4acfd81f5ca36b..cab11b6c4ebede 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -110,7 +110,7 @@ void Lowering::LowerStoreLoc(GenTreeLclVarCommon* storeLoc) // Return Value: // None. // -void Lowering::LowerStoreIndir(GenTreeIndir* node) +void Lowering::LowerStoreIndir(GenTreeStoreInd* node) { // Mark all GT_STOREIND nodes to indicate that it is not known // whether it represents a RMW memory op. @@ -4575,7 +4575,7 @@ void Lowering::ContainCheckIndir(GenTreeIndir* node) // Arguments: // node - pointer to the node // -void Lowering::ContainCheckStoreIndir(GenTreeIndir* node) +void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) { // If the source is a containable immediate, make it contained, unless it is // an int-size or larger store of zero to memory, because we can generate smaller code From 35642851470f5ddd1bb11e234af33460427b7d9a Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 6 Jun 2021 04:11:20 +0300 Subject: [PATCH 2/4] Improve clarity of code through the use of helpers --- src/coreclr/jit/lowerarmarch.cpp | 4 ++-- src/coreclr/jit/lowerxarch.cpp | 12 +++++++----- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/coreclr/jit/lowerarmarch.cpp b/src/coreclr/jit/lowerarmarch.cpp index 5571f89a7c9631..8af5527b6dfe72 100644 --- a/src/coreclr/jit/lowerarmarch.cpp +++ b/src/coreclr/jit/lowerarmarch.cpp @@ -1379,8 +1379,8 @@ void Lowering::ContainCheckCallOperands(GenTreeCall* call) void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) { #ifdef TARGET_ARM64 - GenTree* src = node->AsOp()->gtOp2; - if (!varTypeIsFloating(src->TypeGet()) && src->IsIntegralConst(0)) + GenTree* src = node->Data(); + if (src->IsIntegralConst(0)) { // an integer zero for 'src' can be contained. MakeSrcContained(node, src); diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index cab11b6c4ebede..37811d03bb0fff 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -114,7 +114,7 @@ void Lowering::LowerStoreIndir(GenTreeStoreInd* node) { // Mark all GT_STOREIND nodes to indicate that it is not known // whether it represents a RMW memory op. - node->AsStoreInd()->SetRMWStatusDefault(); + node->SetRMWStatusDefault(); if (!varTypeIsFloating(node)) { @@ -130,10 +130,10 @@ void Lowering::LowerStoreIndir(GenTreeStoreInd* node) return; } } - else if (node->AsStoreInd()->Data()->OperIs(GT_CNS_DBL)) + else if (node->Data()->IsCnsFltOrDbl()) { // Optimize *x = DCON to *x = ICON which is slightly faster on xarch - GenTree* data = node->AsStoreInd()->Data(); + GenTree* data = node->Data(); double dblCns = data->AsDblCon()->gtDconVal; ssize_t intCns = 0; var_types type = TYP_UNKNOWN; @@ -162,6 +162,7 @@ void Lowering::LowerStoreIndir(GenTreeStoreInd* node) node->ChangeType(type); } } + ContainCheckStoreIndir(node); } @@ -4580,12 +4581,13 @@ void Lowering::ContainCheckStoreIndir(GenTreeStoreInd* node) // If the source is a containable immediate, make it contained, unless it is // an int-size or larger store of zero to memory, because we can generate smaller code // by zeroing a register and then storing it. - GenTree* src = node->AsOp()->gtOp2; + GenTree* src = node->Data(); if (IsContainableImmed(node, src) && - (!src->IsIntegralConst(0) || varTypeIsSmall(node) || node->gtGetOp1()->OperGet() == GT_CLS_VAR_ADDR)) + (!src->IsIntegralConst(0) || varTypeIsSmall(node) || node->Addr()->OperIs(GT_CLS_VAR_ADDR))) { MakeSrcContained(node, src); } + ContainCheckIndir(node); } From 8014879f0597d551e2b06480c08fdc6286d11442 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 6 Jun 2021 05:55:00 +0300 Subject: [PATCH 3/4] Move the "do not zero-extend setcc" to lowering It is XARCH-specific and moving it eliminates questionable code that is trying to compensate for CSE changing the store. --- src/coreclr/jit/lowerxarch.cpp | 6 ++++++ src/coreclr/jit/morph.cpp | 15 --------------- 2 files changed, 6 insertions(+), 15 deletions(-) diff --git a/src/coreclr/jit/lowerxarch.cpp b/src/coreclr/jit/lowerxarch.cpp index 37811d03bb0fff..62ef614e8e9874 100644 --- a/src/coreclr/jit/lowerxarch.cpp +++ b/src/coreclr/jit/lowerxarch.cpp @@ -163,6 +163,12 @@ void Lowering::LowerStoreIndir(GenTreeStoreInd* node) } } + // Optimization: do not unnecessarily zero-extend the result of setcc. + if (varTypeIsByte(node) && (node->Data()->OperIsCompare() || node->Data()->OperIs(GT_SETCC))) + { + node->Data()->ChangeType(TYP_BYTE); + } + ContainCheckStoreIndir(node); } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index b6e1f808ea9094..e77ab778c1abbc 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13394,23 +13394,8 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) tree->AsOp()->gtOp2 = op2 = op2->AsCast()->CastOp(); } } - else if (op2->OperIsCompare() && varTypeIsByte(effectiveOp1->TypeGet())) - { - /* We don't need to zero extend the setcc instruction */ - op2->gtType = TYP_BYTE; - } } - // If we introduced a CSE we may need to undo the optimization above - // (i.e. " op2->gtType = TYP_BYTE;" which depends upon op1 being a GT_IND of a byte type) - // When we introduce the CSE we remove the GT_IND and subsitute a GT_LCL_VAR in it place. - else if (op2->OperIsCompare() && (op2->gtType == TYP_BYTE) && (op1->gtOper == GT_LCL_VAR)) - { - unsigned varNum = op1->AsLclVarCommon()->GetLclNum(); - LclVarDsc* varDsc = &lvaTable[varNum]; - /* We again need to zero extend the setcc instruction */ - op2->gtType = varDsc->TypeGet(); - } fgAssignSetVarDef(tree); /* We can't CSE the LHS of an assignment */ From 0d79c9a514c7530a493b65b742b9e504bbc9e6b0 Mon Sep 17 00:00:00 2001 From: SingleAccretion Date: Sun, 13 Jun 2021 22:29:06 +0300 Subject: [PATCH 4/4] Delete now unnecessary copying of the relop type --- src/coreclr/jit/morph.cpp | 3 --- 1 file changed, 3 deletions(-) diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index e77ab778c1abbc..5f59206e0babf8 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13637,9 +13637,6 @@ GenTree* Compiler::fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac) gtReverseCond(op1); } - /* Propagate gtType of tree into op1 in case it is TYP_BYTE for setcc optimization */ - op1->gtType = tree->gtType; - noway_assert((op1->gtFlags & GTF_RELOP_JMP_USED) == 0); op1->gtFlags |= tree->gtFlags & (GTF_RELOP_JMP_USED | GTF_RELOP_QMARK | GTF_DONT_CSE);