From 3ddd7c3991805c76e34118772d0ca8f9764c8bcf Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 5 Mar 2023 21:18:07 +0100 Subject: [PATCH 01/19] Stress testing gtSplitTree --- src/coreclr/jit/compiler.cpp | 57 +++++++ src/coreclr/jit/compiler.h | 5 + src/coreclr/jit/compphases.h | 1 + src/coreclr/jit/gentree.cpp | 297 +++++++++++++++++++++++++++++++++++ 4 files changed, 360 insertions(+) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 2856cc56d6b1ef..86530fee16f8e4 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4950,6 +4950,10 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl fgDomsComputed = false; optLoopTableValid = false; +#ifdef DEBUG + DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree); +#endif + // Insert GC Polls DoPhase(this, PHASE_INSERT_GC_POLLS, &Compiler::fgInsertGCPolls); @@ -5291,6 +5295,59 @@ PhaseStatus Compiler::placeLoopAlignInstructions() } #endif +PhaseStatus Compiler::StressSplitTree() +{ + //if (!compStressCompile(STRESS_SPLIT_TREE, 10)) + // return PhaseStatus::MODIFIED_NOTHING; + + CLRRandom rng; + rng.Init(info.compMethodHash() ^ 0x077cc4d4); + + for (BasicBlock* block : Blocks()) + { + for (Statement* stmt : block->NonPhiStatements()) + { + bool skip = false; + int numTrees = 0; + for (GenTree* tree : stmt->TreeList()) + { + if (varTypeIsSIMD(tree)) + { + skip = true; + break; + } + if (!tree->OperIs(GT_JTRUE)) // due to relop invariant + { + numTrees++; + } + } + + if (skip) + continue; + + int splitTree = rng.Next(numTrees); + for (GenTree* tree : stmt->TreeList()) + { + if (splitTree == 0) + { + JITDUMP("Splitting " FMT_STMT " at [%06u]\n", stmt->GetID(), dspTreeID(tree)); + Statement* firstNewStmt; + GenTree** use; + gtSplitTree(block, stmt, tree, &firstNewStmt, &use); + break; + } + + if (!tree->OperIs(GT_JTRUE)) + { + splitTree--; + } + } + } + } + + return PhaseStatus::MODIFIED_EVERYTHING; +} + //------------------------------------------------------------------------ // generatePatchpointInfo: allocate and fill in patchpoint info data, // and report it to the VM diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index a846e040ec33c7..370c9f712117d3 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2929,6 +2929,9 @@ class Compiler GenTreeFlags GenTreeFlags = GTF_SIDE_EFFECT, bool ignoreRoot = false); + void gtSplitTree( + BasicBlock* block, Statement* stmt, GenTree* splitPoint, Statement** firstNewStmt, GenTree*** splitPointUse); + // Static fields of struct types (and sometimes the types that those are reduced to) are represented by having the // static field contain an object pointer to the boxed struct. This simplifies the GC implementation...but // complicates the JIT somewhat. This predicate returns "true" iff a node with type "fieldNodeType", representing @@ -5228,6 +5231,7 @@ class Compiler // Initialize the per-block variable sets (used for liveness analysis). void fgInitBlockVarSets(); + PhaseStatus StressSplitTree(); PhaseStatus fgInsertGCPolls(); BasicBlock* fgCreateGCPoll(GCPollType pollType, BasicBlock* block); @@ -9690,6 +9694,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX STRESS_MODE(PROMOTE_FEWER_STRUCTS)/* Don't promote some structs that can be promoted */ \ STRESS_MODE(VN_BUDGET)/* Randomize the VN budget */ \ STRESS_MODE(SSA_INFO) /* Select lower thresholds for "complex" SSA num encoding */ \ + STRESS_MODE(SPLIT_TREE) \ \ /* After COUNT_VARN, stress level 2 does all of these all the time */ \ \ diff --git a/src/coreclr/jit/compphases.h b/src/coreclr/jit/compphases.h index 880cfe6c4d6a20..c3f96621e80e6f 100644 --- a/src/coreclr/jit/compphases.h +++ b/src/coreclr/jit/compphases.h @@ -91,6 +91,7 @@ CompPhaseNameMacro(PHASE_IF_CONVERSION, "If conversion", CompPhaseNameMacro(PHASE_VN_BASED_DEAD_STORE_REMOVAL,"VN-based dead store removal", false, -1, false) CompPhaseNameMacro(PHASE_OPT_UPDATE_FLOW_GRAPH, "Update flow graph opt pass", false, -1, false) CompPhaseNameMacro(PHASE_COMPUTE_EDGE_WEIGHTS2, "Compute edge weights (2, false)",false, -1, false) +CompPhaseNameMacro(PHASE_STRESS_SPLIT_TREE, "Stress gtSplitTree", false, -1, false) CompPhaseNameMacro(PHASE_INSERT_GC_POLLS, "Insert GC Polls", false, -1, true) CompPhaseNameMacro(PHASE_DETERMINE_FIRST_COLD_BLOCK, "Determine first cold block", false, -1, true) CompPhaseNameMacro(PHASE_RATIONALIZE, "Rationalize IR", false, -1, false) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index bf8a6ef3b92689..384f589dfe38f3 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16212,6 +16212,303 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S return true; } +//------------------------------------------------------------------------ +// gtSplitTree: Split a statement into multiple statements such that a +// specified tree is the first executed non-invariant node in the statement. +// +// Arguments: +// block - The block containing the statement. +// stmt - The statement containing the tree. +// splitPoint - A tree inside the statement. +// firstNewStmt - [out] The first new statement that was introduced. +// [firstNewStmt..stmt) are the statements added by this function. +// splitNodeUse - The use of the tree to split at. +// +// Notes: +// This method turns all non-invariant nodes that would be executed before +// the split point into new separate statements. If those nodes were values +// this involves introducing new locals for those values, such that they can +// be used in the original statement. +// +void Compiler::gtSplitTree( + BasicBlock* block, Statement* stmt, GenTree* splitPoint, Statement** firstNewStmt, GenTree*** splitNodeUse) +{ + class Splitter final : public GenTreeVisitor + { + BasicBlock* m_bb; + Statement* m_splitStmt; + GenTree* m_splitNode; + + struct UseInfo + { + GenTree** Use; + GenTree* User; + }; + ArrayStack m_useStack; + + public: + enum + { + DoPreOrder = true, + DoPostOrder = true, + UseExecutionOrder = true + }; + + Splitter(Compiler* compiler, BasicBlock* bb, Statement* stmt, GenTree* splitNode) + : GenTreeVisitor(compiler) + , m_bb(bb) + , m_splitStmt(stmt) + , m_splitNode(splitNode) + , m_useStack(compiler->getAllocator(CMK_ArrayStack)) + { + } + + Statement* FirstStatement = nullptr; + GenTree** SplitNodeUse = nullptr; + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + assert(!(*use)->OperIs(GT_QMARK)); + m_useStack.Push(UseInfo{use, user}); + return WALK_CONTINUE; + } + + fgWalkResult PostOrderVisit(GenTree** use, GenTree* user) + { + if (*use == m_splitNode) + { + bool userIsLocation = false; + // Split all siblings and ancestor siblings. + int i; + for (i = 0; i < m_useStack.Height() - 1; i++) + { + const UseInfo& useInf = m_useStack.BottomRef(i); + if (useInf.Use == use) + { + break; + } + + // If this has the same user as the next node then it is a + // sibling of an ancestor -- and thus not on the "path" + // that contains the split node. + if (m_useStack.BottomRef(i + 1).User == useInf.User) + { + SplitOutUse(useInf, IsLocation(useInf, userIsLocation)); + } + else + { + // This is an ancestor of the node we're splitting on. + userIsLocation = IsLocation(useInf, userIsLocation); + } + } + + assert(m_useStack.Bottom(i).Use == use); + userIsLocation = IsLocation(m_useStack.BottomRef(i), userIsLocation); + + // The remaining nodes should be operands of the split node. + for (i++; i < m_useStack.Height(); i++) + { + const UseInfo& useInf = m_useStack.BottomRef(i); + assert(useInf.User == *use); + bool isLocation = IsLocation(useInf, userIsLocation); + SplitOutUse(useInf, isLocation); + } + + SplitNodeUse = use; + + return WALK_ABORT; + } + + while (m_useStack.Top(0).Use != use) + { + m_useStack.Pop(); + } + + return WALK_CONTINUE; + } + + private: + bool IsLocation(const UseInfo& useInf, bool userIsLocation) + { + if (useInf.User != nullptr) + { + if (useInf.User->OperIs(GT_ADDR, GT_ASG) && (useInf.Use == &useInf.User->AsUnOp()->gtOp1)) + { + return true; + } + + if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) && (useInf.Use == &useInf.User->AsStoreDynBlk()->Data())) + { + return true; + } + + if (userIsLocation && useInf.User->OperIs(GT_COMMA) && (useInf.Use == &useInf.User->AsOp()->gtOp2)) + { + return true; + } + } + + return false; + } + + void SplitOutUse(const UseInfo& useInf, bool isLocation) + { + GenTree** use = useInf.Use; + GenTree* user = useInf.User; + + if ((*use)->IsInvariant()) + { + return; + } + + if ((*use)->OperIs(GT_LCL_VAR) && !m_compiler->lvaGetDesc((*use)->AsLclVarCommon())->IsAddressExposed()) + { + // The splitting we do here should always guarantee that we + // only introduce locals for the tree edges that overlap the + // split point, so it should be ok to avoid creating statements + // for locals that aren't address exposed. Note that this + // relies on it being illegal IR to have a tree edge for a + // register candidate that overlaps with an interfering node. + // + // For example, this optimization would be problematic if IR + // like the following could occur: + // + // CALL + // LCL_VAR V00 + // CALL + // ASG(V00, ...) (setup) + // LCL_VAR V00 + // + return; + } + + if (isLocation) + { + if ((*use)->OperIs(GT_COMMA)) + { + // We have: + // User + // COMMA + // op1 + // op2 + // rhs + // where the comma is a location, and we want to split it out. + // + // The first use will be the COMMA --- op1 edge, which we + // expect to be handled by simple side effect extraction in + // the recursive call. + UseInfo use1{&(*use)->AsOp()->gtOp1, *use}; + + // For the second use we will update the user to use op2 + // directly instead of the comma so that we get the proper + // location treatment. The edge will then be the User --- + // op2 edge. + *use = (*use)->gtGetOp2(); + UseInfo use2{use, user}; + + if ((*use)->IsReverseOp()) + { + SplitOutUse(use2, true); + SplitOutUse(use1, false); + } + else + { + SplitOutUse(use1, false); + SplitOutUse(use2, true); + } + return; + } + + // Only a handful of nodes can be location, and htey are all unary or nullary. + assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD)); + if ((*use)->OperIsUnary()) + { + user = *use; + use = &(*use)->AsUnOp()->gtOp1; + } + else + { + return; + } + } + + Statement* stmt = nullptr; + if (!(*use)->IsValue() || (*use)->OperIs(GT_ASG) || (user == nullptr) || + (user->OperIs(GT_COMMA) && (user->gtGetOp1() == *use))) + { + GenTree* sideEffects = nullptr; + m_compiler->gtExtractSideEffList(*use, &sideEffects); + if (sideEffects != nullptr) + { + stmt = m_compiler->fgNewStmtFromTree(sideEffects, m_splitStmt->GetDebugInfo()); + } + *use = m_compiler->gtNewNothingNode(); + } + else if ((*use)->OperIs(GT_FIELD_LIST, GT_INIT_VAL)) + { + for (GenTree** operandUse : (*use)->UseEdges()) + { + SplitOutUse(UseInfo{ operandUse, *use }, false); + } + return; + } + else + { + unsigned lclNum = m_compiler->lvaGrabTemp(true DEBUGARG("Spilling to split statement for tree")); + if ((*use)->TypeIs(TYP_STRUCT)) + { + ClassLayout* layout = (*use)->GetLayout(m_compiler); + assert(layout != nullptr); + if (layout->IsBlockLayout()) + { + LclVarDsc* dsc = m_compiler->lvaGetDesc(lclNum); + dsc->lvType = TYP_BLK; + dsc->lvExactSize = max(layout->GetSize(), 1); + m_compiler->lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE)); + + GenTreeLclFld* fldDst = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); + fldDst->SetLayout(layout); + GenTree* asg = m_compiler->gtNewAssignNode(fldDst, *use); + stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); + + GenTreeLclFld* fldSrc = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); + fldSrc->SetLayout(layout); + *use = fldSrc; + } + } + + if (stmt == nullptr) + { + GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); + stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); + *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); + } + } + + if (stmt != nullptr) + { + if (FirstStatement == nullptr) + { + FirstStatement = stmt; + } + m_compiler->gtUpdateStmtSideEffects(stmt); + m_compiler->gtSetStmtInfo(stmt); + m_compiler->fgSetStmtSeq(stmt); + m_compiler->fgInsertStmtBefore(m_bb, m_splitStmt, stmt); + } + } + }; + + Splitter splitter(this, block, stmt, splitPoint); + splitter.WalkTree(stmt->GetRootNodePointer(), nullptr); + *firstNewStmt = splitter.FirstStatement; + *splitNodeUse = splitter.SplitNodeUse; + + gtUpdateStmtSideEffects(stmt); + gtSetStmtInfo(stmt); + fgSetStmtSeq(stmt); +} + //------------------------------------------------------------------------ // gtExtractSideEffList: Extracts side effects from the given expression. // From 62909897586a98ab6af370ed172d7964baf1080b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 5 Mar 2023 21:24:38 +0100 Subject: [PATCH 02/19] Misc --- src/coreclr/jit/compiler.cpp | 24 ++++++++++++++---------- 1 file changed, 14 insertions(+), 10 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 86530fee16f8e4..d6c2f0e082d2aa 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5297,8 +5297,8 @@ PhaseStatus Compiler::placeLoopAlignInstructions() PhaseStatus Compiler::StressSplitTree() { - //if (!compStressCompile(STRESS_SPLIT_TREE, 10)) - // return PhaseStatus::MODIFIED_NOTHING; + if (opts.OptimizationDisabled()) + return PhaseStatus::MODIFIED_NOTHING; CLRRandom rng; rng.Init(info.compMethodHash() ^ 0x077cc4d4); @@ -5311,15 +5311,19 @@ PhaseStatus Compiler::StressSplitTree() int numTrees = 0; for (GenTree* tree : stmt->TreeList()) { + if (tree->OperIs(GT_JTRUE)) + { + continue; + } + if (varTypeIsSIMD(tree)) { + // Cannot always create locals for these due to bug in gtGetStructHandleForSIMD skip = true; break; } - if (!tree->OperIs(GT_JTRUE)) // due to relop invariant - { - numTrees++; - } + + numTrees++; } if (skip) @@ -5328,6 +5332,9 @@ PhaseStatus Compiler::StressSplitTree() int splitTree = rng.Next(numTrees); for (GenTree* tree : stmt->TreeList()) { + if (tree->OperIs(GT_JTRUE)) + continue; + if (splitTree == 0) { JITDUMP("Splitting " FMT_STMT " at [%06u]\n", stmt->GetID(), dspTreeID(tree)); @@ -5337,10 +5344,7 @@ PhaseStatus Compiler::StressSplitTree() break; } - if (!tree->OperIs(GT_JTRUE)) - { - splitTree--; - } + splitTree--; } } } From 1a366779513b1696a241d9d6ece2d2374b272ce9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 5 Mar 2023 21:26:17 +0100 Subject: [PATCH 03/19] Misc fixes --- src/coreclr/jit/compiler.cpp | 11 +++++------ src/coreclr/jit/gentree.cpp | 15 ++++++++------- 2 files changed, 13 insertions(+), 13 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index d6c2f0e082d2aa..5a95513de84afe 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5297,9 +5297,7 @@ PhaseStatus Compiler::placeLoopAlignInstructions() PhaseStatus Compiler::StressSplitTree() { - if (opts.OptimizationDisabled()) - return PhaseStatus::MODIFIED_NOTHING; - +#ifdef DEBUG CLRRandom rng; rng.Init(info.compMethodHash() ^ 0x077cc4d4); @@ -5307,8 +5305,8 @@ PhaseStatus Compiler::StressSplitTree() { for (Statement* stmt : block->NonPhiStatements()) { - bool skip = false; - int numTrees = 0; + bool skip = false; + int numTrees = 0; for (GenTree* tree : stmt->TreeList()) { if (tree->OperIs(GT_JTRUE)) @@ -5339,7 +5337,7 @@ PhaseStatus Compiler::StressSplitTree() { JITDUMP("Splitting " FMT_STMT " at [%06u]\n", stmt->GetID(), dspTreeID(tree)); Statement* firstNewStmt; - GenTree** use; + GenTree** use; gtSplitTree(block, stmt, tree, &firstNewStmt, &use); break; } @@ -5348,6 +5346,7 @@ PhaseStatus Compiler::StressSplitTree() } } } +#endif return PhaseStatus::MODIFIED_EVERYTHING; } diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 384f589dfe38f3..f3b550f88bf468 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16337,7 +16337,8 @@ void Compiler::gtSplitTree( return true; } - if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) && (useInf.Use == &useInf.User->AsStoreDynBlk()->Data())) + if (useInf.User->OperIs(GT_STORE_DYN_BLK) && !(*useInf.Use)->OperIs(GT_CNS_INT, GT_INIT_VAL) && + (useInf.Use == &useInf.User->AsStoreDynBlk()->Data())) { return true; } @@ -16448,7 +16449,7 @@ void Compiler::gtSplitTree( { for (GenTree** operandUse : (*use)->UseEdges()) { - SplitOutUse(UseInfo{ operandUse, *use }, false); + SplitOutUse(UseInfo{operandUse, *use}, false); } return; } @@ -16461,15 +16462,15 @@ void Compiler::gtSplitTree( assert(layout != nullptr); if (layout->IsBlockLayout()) { - LclVarDsc* dsc = m_compiler->lvaGetDesc(lclNum); - dsc->lvType = TYP_BLK; + LclVarDsc* dsc = m_compiler->lvaGetDesc(lclNum); + dsc->lvType = TYP_BLK; dsc->lvExactSize = max(layout->GetSize(), 1); m_compiler->lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE)); GenTreeLclFld* fldDst = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); fldDst->SetLayout(layout); GenTree* asg = m_compiler->gtNewAssignNode(fldDst, *use); - stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); + stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); GenTreeLclFld* fldSrc = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); fldSrc->SetLayout(layout); @@ -16480,8 +16481,8 @@ void Compiler::gtSplitTree( if (stmt == nullptr) { GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); - stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); - *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); + stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); + *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); } } From 261553726cd749eda096e7e91e28cd3bb56cc0df Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sun, 5 Mar 2023 22:22:41 +0100 Subject: [PATCH 04/19] Handle GT_MUL with GTF_MUL_64RSLT --- src/coreclr/jit/gentree.cpp | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index f3b550f88bf468..fafad6dbb9ffc0 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16433,6 +16433,18 @@ void Compiler::gtSplitTree( } } +#ifndef TARGET_64BIT + // GT_MUL with GTF_MUL_64RSLT is required to stay with casts on the + // operands. Note that one operand may also be a constant, but we + // would have exited early above for that case. + if ((user != nullptr) && user->OperIs(GT_MUL) && user->Is64RsltMul()) + { + assert((*use)->OperIs(GT_CAST)); + user = *use; + use = &(*use)->AsCast()->gtOp1; + } +#endif + Statement* stmt = nullptr; if (!(*use)->IsValue() || (*use)->OperIs(GT_ASG) || (user == nullptr) || (user->OperIs(GT_COMMA) && (user->gtGetOp1() == *use))) From b5038bae4f2062f950636064b0c657222b5cef90 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 6 Mar 2023 00:20:32 +0100 Subject: [PATCH 05/19] Morph modified statement with locals --- src/coreclr/jit/gentree.cpp | 1 + 1 file changed, 1 insertion(+) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index fafad6dbb9ffc0..5d736bb8591daa 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16520,6 +16520,7 @@ void Compiler::gtSplitTree( gtUpdateStmtSideEffects(stmt); gtSetStmtInfo(stmt); fgSetStmtSeq(stmt); + fgMorphBlockStmt(block, stmt DEBUGARG("Original statement")); } //------------------------------------------------------------------------ From 5a9aa91248b819a17c81d5fb2044372759a3ee07 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 6 Mar 2023 00:21:09 +0100 Subject: [PATCH 06/19] Clean up a bit --- src/coreclr/jit/gentree.cpp | 17 ++++++++++++----- 1 file changed, 12 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 5d736bb8591daa..4a186c7458fda7 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16265,6 +16265,7 @@ void Compiler::gtSplitTree( Statement* FirstStatement = nullptr; GenTree** SplitNodeUse = nullptr; + bool MadeChanges = false; fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) { @@ -16456,6 +16457,7 @@ void Compiler::gtSplitTree( stmt = m_compiler->fgNewStmtFromTree(sideEffects, m_splitStmt->GetDebugInfo()); } *use = m_compiler->gtNewNothingNode(); + MadeChanges = true; } else if ((*use)->OperIs(GT_FIELD_LIST, GT_INIT_VAL)) { @@ -16468,6 +16470,7 @@ void Compiler::gtSplitTree( else { unsigned lclNum = m_compiler->lvaGrabTemp(true DEBUGARG("Spilling to split statement for tree")); + if ((*use)->TypeIs(TYP_STRUCT)) { ClassLayout* layout = (*use)->GetLayout(m_compiler); @@ -16487,6 +16490,7 @@ void Compiler::gtSplitTree( GenTreeLclFld* fldSrc = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); fldSrc->SetLayout(layout); *use = fldSrc; + MadeChanges = true; } } @@ -16495,6 +16499,7 @@ void Compiler::gtSplitTree( GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); + MadeChanges = true; } } @@ -16504,7 +16509,7 @@ void Compiler::gtSplitTree( { FirstStatement = stmt; } - m_compiler->gtUpdateStmtSideEffects(stmt); + m_compiler->gtSetStmtInfo(stmt); m_compiler->fgSetStmtSeq(stmt); m_compiler->fgInsertStmtBefore(m_bb, m_splitStmt, stmt); @@ -16517,10 +16522,12 @@ void Compiler::gtSplitTree( *firstNewStmt = splitter.FirstStatement; *splitNodeUse = splitter.SplitNodeUse; - gtUpdateStmtSideEffects(stmt); - gtSetStmtInfo(stmt); - fgSetStmtSeq(stmt); - fgMorphBlockStmt(block, stmt DEBUGARG("Original statement")); + if (splitter.MadeChanges) + { + gtUpdateStmtSideEffects(stmt); + // We may have introduced new LCL_VAR struct uses that need to go through block morphing. + fgMorphBlockStmt(block, stmt DEBUGARG("gtSplitTree")); + } } //------------------------------------------------------------------------ From 776b40307466032d958cc50f4787f4c52b1f8257 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Mon, 6 Mar 2023 10:50:12 +0100 Subject: [PATCH 07/19] Multi-reg returns --- src/coreclr/jit/gentree.cpp | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 4a186c7458fda7..7675083f2f8387 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16494,6 +16494,13 @@ void Compiler::gtSplitTree( } } + if (varTypeIsStruct(*use) && + ((*use)->IsMultiRegNode() || + ((user != nullptr) && user->OperIs(GT_RETURN) && m_compiler->compMethodReturnsMultiRegRetType()))) + { + m_compiler->lvaGetDesc(lclNum)->lvIsMultiRegRet = true; + } + if (stmt == nullptr) { GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); From 0c70dcb9874bc3504a73e2e8ce7d4f9c1b9ee1c9 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Mar 2023 19:53:47 +0100 Subject: [PATCH 08/19] JIT: Disallow 0-sized block ops Fix #12807 --- src/coreclr/jit/gentree.h | 1 + src/coreclr/jit/importer.cpp | 61 +++++++++++++++------------------- src/coreclr/jit/morphblock.cpp | 15 ++------- 3 files changed, 30 insertions(+), 47 deletions(-) diff --git a/src/coreclr/jit/gentree.h b/src/coreclr/jit/gentree.h index 9972b71d8b8c54..57eeaaf8f6fb0d 100644 --- a/src/coreclr/jit/gentree.h +++ b/src/coreclr/jit/gentree.h @@ -7326,6 +7326,7 @@ struct GenTreeBlk : public GenTreeIndir void Initialize(ClassLayout* layout) { assert(OperIsBlk(OperGet()) && ((layout != nullptr) || OperIs(GT_STORE_DYN_BLK))); + assert((layout == nullptr) || (layout->GetSize() != 0)); m_layout = layout; gtBlkOpKind = BlkOpKindInvalid; diff --git a/src/coreclr/jit/importer.cpp b/src/coreclr/jit/importer.cpp index f09909d0597829..5734f49d99ed6e 100644 --- a/src/coreclr/jit/importer.cpp +++ b/src/coreclr/jit/importer.cpp @@ -10645,56 +10645,47 @@ void Compiler::impImportBlockCode(BasicBlock* block) } case CEE_INITBLK: + case CEE_CPBLK: op3 = impPopStack().val; // Size - op2 = impPopStack().val; // Value + op2 = impPopStack().val; // Value / Src addr op1 = impPopStack().val; // Dst addr if (op3->IsCnsIntOrI()) { - size = (unsigned)op3->AsIntConCommon()->IconValue(); - op1 = new (this, GT_BLK) GenTreeBlk(GT_BLK, TYP_STRUCT, op1, typGetBlkLayout(size)); - op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0); - } - else - { - if (!op2->IsIntegralConst(0)) + if (op3->IsIntegralConst(0)) { - op2 = gtNewOperNode(GT_INIT_VAL, TYP_INT, op2); - } - -#ifdef TARGET_64BIT - // STORE_DYN_BLK takes a native uint size as it turns into call to memset. - op3 = gtNewCastNode(TYP_I_IMPL, op3, /* fromUnsigned */ true, TYP_U_IMPL); -#endif + if ((op1->gtFlags & GTF_SIDE_EFFECT) != 0) + { + impAppendTree(gtUnusedValNode(op1), CHECK_SPILL_ALL, impCurStmtDI); + } - op1 = new (this, GT_STORE_DYN_BLK) GenTreeStoreDynBlk(op1, op2, op3); - size = 0; + if ((op2->gtFlags & GTF_SIDE_EFFECT) != 0) + { + impAppendTree(gtUnusedValNode(op2), CHECK_SPILL_ALL, impCurStmtDI); + } - if ((prefixFlags & PREFIX_VOLATILE) != 0) - { - op1->gtFlags |= GTF_BLK_VOLATILE; + break; } - } - goto SPILL_APPEND; - - case CEE_CPBLK: - - op3 = impPopStack().val; // Size - op2 = impPopStack().val; // Src addr - op1 = impPopStack().val; // Dst addr - if (op3->IsCnsIntOrI()) - { size = static_cast(op3->AsIntConCommon()->IconValue()); - - op1 = gtNewBlockVal(op1, size); - op2 = gtNewBlockVal(op2, size); - op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0); + op1 = gtNewBlockVal(op1, size); + op2 = opcode == CEE_INITBLK ? op2 : gtNewBlockVal(op2, size); + op1 = gtNewBlkOpNode(op1, op2, (prefixFlags & PREFIX_VOLATILE) != 0); } else { - op2 = gtNewOperNode(GT_IND, TYP_STRUCT, op2); + if (opcode == CEE_INITBLK) + { + if (!op2->IsIntegralConst(0)) + { + op2 = gtNewOperNode(GT_INIT_VAL, TYP_INT, op2); + } + } + else + { + op2 = gtNewOperNode(GT_IND, TYP_STRUCT, op2); + } #ifdef TARGET_64BIT // STORE_DYN_BLK takes a native uint size as it turns into call to memcpy. diff --git a/src/coreclr/jit/morphblock.cpp b/src/coreclr/jit/morphblock.cpp index 272c31edca0769..f26da00cc42ce5 100644 --- a/src/coreclr/jit/morphblock.cpp +++ b/src/coreclr/jit/morphblock.cpp @@ -211,6 +211,8 @@ void MorphInitBlockHelper::PrepareDst() m_blockSize = genTypeSize(m_dst); } + assert(m_blockSize != 0); + #if defined(DEBUG) if (m_comp->verbose) { @@ -481,12 +483,6 @@ void MorphInitBlockHelper::TryInitFieldByField() LclVarDsc* destLclVar = m_dstVarDsc; unsigned blockSize = m_blockSize; - if (blockSize == 0) - { - JITDUMP(" size is zero.\n"); - return; - } - if (destLclVar->IsAddressExposed() && destLclVar->lvContainsHoles) { JITDUMP(" dest is address exposed and contains holes.\n"); @@ -647,11 +643,6 @@ void MorphInitBlockHelper::TryInitFieldByField() // void MorphInitBlockHelper::TryPrimitiveInit() { - if (m_blockSize == 0) - { - return; - } - if (m_src->IsIntegralConst(0) && (m_dstVarDsc != nullptr) && (genTypeSize(m_dstVarDsc) == m_blockSize)) { var_types lclVarType = m_dstVarDsc->TypeGet(); @@ -1101,7 +1092,7 @@ void MorphCopyBlockHelper::MorphStructCases() // void MorphCopyBlockHelper::TryPrimitiveCopy() { - if (!m_dst->TypeIs(TYP_STRUCT) || (m_blockSize == 0)) + if (!m_dst->TypeIs(TYP_STRUCT)) { return; } From 2a8644f365d45a692e1faf2102edd668986ef4f1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Mar 2023 21:29:37 +0100 Subject: [PATCH 09/19] Remove TYP_BLK special case --- src/coreclr/jit/gentree.cpp | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b4104ba5fae5ee..98fa6154921923 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16442,29 +16442,6 @@ void Compiler::gtSplitTree( { unsigned lclNum = m_compiler->lvaGrabTemp(true DEBUGARG("Spilling to split statement for tree")); - if ((*use)->TypeIs(TYP_STRUCT)) - { - ClassLayout* layout = (*use)->GetLayout(m_compiler); - assert(layout != nullptr); - if (layout->IsBlockLayout()) - { - LclVarDsc* dsc = m_compiler->lvaGetDesc(lclNum); - dsc->lvType = TYP_BLK; - dsc->lvExactSize = max(layout->GetSize(), 1); - m_compiler->lvaSetVarAddrExposed(lclNum DEBUGARG(AddressExposedReason::TOO_CONSERVATIVE)); - - GenTreeLclFld* fldDst = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); - fldDst->SetLayout(layout); - GenTree* asg = m_compiler->gtNewAssignNode(fldDst, *use); - stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); - - GenTreeLclFld* fldSrc = m_compiler->gtNewLclFldNode(lclNum, TYP_STRUCT, 0); - fldSrc->SetLayout(layout); - *use = fldSrc; - MadeChanges = true; - } - } - if (varTypeIsStruct(*use) && ((*use)->IsMultiRegNode() || ((user != nullptr) && user->OperIs(GT_RETURN) && m_compiler->compMethodReturnsMultiRegRetType()))) From 57e875c1bfcadba343098b0ad110f56f5512a04b Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Mar 2023 22:00:24 +0100 Subject: [PATCH 10/19] Remove SIMD special case --- src/coreclr/jit/compiler.cpp | 13 +------------ 1 file changed, 1 insertion(+), 12 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 76f9609de3f5d6..04d422265bf8fb 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5351,28 +5351,17 @@ PhaseStatus Compiler::StressSplitTree() { for (Statement* stmt : block->NonPhiStatements()) { - bool skip = false; int numTrees = 0; for (GenTree* tree : stmt->TreeList()) { - if (tree->OperIs(GT_JTRUE)) + if (tree->OperIs(GT_JTRUE)) // Due to relop invariant { continue; } - if (varTypeIsSIMD(tree)) - { - // Cannot always create locals for these due to bug in gtGetStructHandleForSIMD - skip = true; - break; - } - numTrees++; } - if (skip) - continue; - int splitTree = rng.Next(numTrees); for (GenTree* tree : stmt->TreeList()) { From 79402271e6b2221c06201e001b8648d0f42dad59 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Thu, 9 Mar 2023 22:01:06 +0100 Subject: [PATCH 11/19] Run jit-format --- src/coreclr/jit/compiler.cpp | 2 +- src/coreclr/jit/gentree.cpp | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 04d422265bf8fb..4f80e5f2b8db86 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5351,7 +5351,7 @@ PhaseStatus Compiler::StressSplitTree() { for (Statement* stmt : block->NonPhiStatements()) { - int numTrees = 0; + int numTrees = 0; for (GenTree* tree : stmt->TreeList()) { if (tree->OperIs(GT_JTRUE)) // Due to relop invariant diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 98fa6154921923..8a7e93de2e7285 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16236,7 +16236,7 @@ void Compiler::gtSplitTree( Statement* FirstStatement = nullptr; GenTree** SplitNodeUse = nullptr; - bool MadeChanges = false; + bool MadeChanges = false; fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) { @@ -16413,7 +16413,7 @@ void Compiler::gtSplitTree( { assert((*use)->OperIs(GT_CAST)); user = *use; - use = &(*use)->AsCast()->gtOp1; + use = &(*use)->AsCast()->gtOp1; } #endif @@ -16427,7 +16427,7 @@ void Compiler::gtSplitTree( { stmt = m_compiler->fgNewStmtFromTree(sideEffects, m_splitStmt->GetDebugInfo()); } - *use = m_compiler->gtNewNothingNode(); + *use = m_compiler->gtNewNothingNode(); MadeChanges = true; } else if ((*use)->OperIs(GT_FIELD_LIST, GT_INIT_VAL)) @@ -16454,7 +16454,7 @@ void Compiler::gtSplitTree( GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); - MadeChanges = true; + MadeChanges = true; } } From c1dcff17bb251bb25731cc461d7ed2ca5e170760 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 10 Mar 2023 15:50:49 +0100 Subject: [PATCH 12/19] Add stress mode for commas, rework handling for block ops Also fix LIR links traversed initialization --- src/coreclr/jit/compiler.cpp | 119 ++++++++++++++++++++++++++++++++-- src/coreclr/jit/compiler.h | 10 ++- src/coreclr/jit/fgstmt.cpp | 2 +- src/coreclr/jit/gentree.cpp | 45 +++++++------ src/coreclr/jit/morph.cpp | 54 +++++++++++++++ src/coreclr/jit/rationalize.h | 3 - 6 files changed, 199 insertions(+), 34 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 4f80e5f2b8db86..8370a284f6501f 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -1776,6 +1776,7 @@ void Compiler::compInit(ArenaAllocator* pAlloc, // set this early so we can use it without relying on random memory values verbose = compIsForInlining() ? impInlineInfo->InlinerCompiler->verbose : false; + compNumStatementLinksTraversed = 0; compPoisoningAnyImplicitByrefs = false; #endif @@ -4996,9 +4997,7 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl fgDomsComputed = false; optLoopTableValid = false; -#ifdef DEBUG DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree); -#endif // Insert GC Polls DoPhase(this, PHASE_INSERT_GC_POLLS, &Compiler::fgInsertGCPolls); @@ -5341,9 +5340,37 @@ PhaseStatus Compiler::placeLoopAlignInstructions() } #endif +//------------------------------------------------------------------------ +// StressSplitTree: A phase that stresses the gtSplitTree function. +// +// Returns: +// Suitable phase status +// +// Notes: +// Stress is applied on a function-by-function basis +// PhaseStatus Compiler::StressSplitTree() { -#ifdef DEBUG + if (compStressCompile(STRESS_SPLIT_TREES_RANDOMLY, 10)) + { + SplitTreesRandomly(); + return PhaseStatus::MODIFIED_EVERYTHING; + } + + if (compStressCompile(STRESS_SPLIT_TREES_REMOVE_COMMAS, 10)) + { + SplitTreesRemoveCommas(); + return PhaseStatus::MODIFIED_EVERYTHING; + } + + return PhaseStatus::MODIFIED_NOTHING; +} + +//------------------------------------------------------------------------ +// SplitTreesRandomly: Split all statements at a random location. +// +void Compiler::SplitTreesRandomly() +{ CLRRandom rng; rng.Init(info.compMethodHash() ^ 0x077cc4d4); @@ -5371,9 +5398,19 @@ PhaseStatus Compiler::StressSplitTree() if (splitTree == 0) { JITDUMP("Splitting " FMT_STMT " at [%06u]\n", stmt->GetID(), dspTreeID(tree)); - Statement* firstNewStmt; + Statement* newStmt; GenTree** use; - gtSplitTree(block, stmt, tree, &firstNewStmt, &use); + if (gtSplitTree(block, stmt, tree, &newStmt, &use)) + { + while ((newStmt != nullptr) && (newStmt != stmt)) + { + fgMorphStmtBlockOps(block, newStmt); + newStmt = newStmt->GetNextStmt(); + } + + fgMorphStmtBlockOps(block, stmt); + } + break; } @@ -5381,9 +5418,77 @@ PhaseStatus Compiler::StressSplitTree() } } } -#endif +} + +//------------------------------------------------------------------------ +// SplitTreesRemoveCommas: Split trees to remove all commas. +// +void Compiler::SplitTreesRemoveCommas() +{ + for (BasicBlock* block : Blocks()) + { + for (Statement* stmt : block->NonPhiStatements()) + { + while (true) + { + bool again = false; + for (GenTree* tree : stmt->TreeList()) + { + if (!tree->OperIs(GT_COMMA)) + { + continue; + } + + // Supporting this weird construct would require additional + // handling, we need to sort of move the comma into to the + // next node in execution order. We don't see this so just + // skip it. + assert(!tree->IsReverseOp()); + + JITDUMP("Removing COMMA [%06u]\n", dspTreeID(tree)); + Statement* newStmt; + GenTree** use; + gtSplitTree(block, stmt, tree, &newStmt, &use); + GenTree* op1SideEffects = nullptr; + gtExtractSideEffList(tree->gtGetOp1(), &op1SideEffects); + + if (op1SideEffects != nullptr) + { + fgInsertStmtBefore(block, stmt, fgNewStmtFromTree(op1SideEffects)); + } + + *use = tree->gtGetOp2(); + + while ((newStmt != nullptr) && (newStmt != stmt)) + { + fgMorphStmtBlockOps(block, newStmt); + newStmt = newStmt->GetNextStmt(); + } + + fgMorphStmtBlockOps(block, stmt); + + again = true; + break; + } + + if (!again) + { + break; + } + } + } + } - return PhaseStatus::MODIFIED_EVERYTHING; + for (BasicBlock* block : Blocks()) + { + for (Statement* stmt : block->NonPhiStatements()) + { + for (GenTree* tree : stmt->TreeList()) + { + assert(!tree->OperIs(GT_COMMA)); + } + } + } } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index b898ee38ee888d..4f3feb1b7e86af 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -2976,7 +2976,7 @@ class Compiler GenTreeFlags GenTreeFlags = GTF_SIDE_EFFECT, bool ignoreRoot = false); - void gtSplitTree( + bool gtSplitTree( BasicBlock* block, Statement* stmt, GenTree* splitPoint, Statement** firstNewStmt, GenTree*** splitPointUse); // Static fields of struct types (and sometimes the types that those are reduced to) are represented by having the @@ -4736,6 +4736,7 @@ class Compiler void fgMergeBlockReturn(BasicBlock* block); bool fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(const char* msg)); + void fgMorphStmtBlockOps(BasicBlock* block, Statement* stmt); //------------------------------------------------------------------------------------------------------------ // MorphMDArrayTempCache: a simple cache of compiler temporaries in the local variable table, used to minimize @@ -5279,6 +5280,8 @@ class Compiler void fgInitBlockVarSets(); PhaseStatus StressSplitTree(); + void SplitTreesRandomly(); + void SplitTreesRemoveCommas(); PhaseStatus fgInsertGCPolls(); BasicBlock* fgCreateGCPoll(GCPollType pollType, BasicBlock* block); @@ -5918,9 +5921,11 @@ class Compiler CORINFO_CONTEXT_HANDLE* ExactContextHnd, methodPointerInfo* ldftnToken); GenTree* fgMorphLeaf(GenTree* tree); +public: GenTree* fgMorphInitBlock(GenTree* tree); GenTree* fgMorphCopyBlock(GenTree* tree); GenTree* fgMorphStoreDynBlock(GenTreeStoreDynBlk* tree); +private: GenTree* fgMorphSmpOp(GenTree* tree, MorphAddrContext* mac, bool* optAssertionPropDone = nullptr); void fgTryReplaceStructLocalWithField(GenTree* tree); GenTree* fgOptimizeCast(GenTreeCast* cast); @@ -9793,7 +9798,8 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX STRESS_MODE(PROMOTE_FEWER_STRUCTS)/* Don't promote some structs that can be promoted */ \ STRESS_MODE(VN_BUDGET)/* Randomize the VN budget */ \ STRESS_MODE(SSA_INFO) /* Select lower thresholds for "complex" SSA num encoding */ \ - STRESS_MODE(SPLIT_TREE) \ + STRESS_MODE(SPLIT_TREES_RANDOMLY) /* Split all statements at a random tree */ \ + STRESS_MODE(SPLIT_TREES_REMOVE_COMMAS) /* Remove all GT_COMMA nodes */ \ \ /* After COUNT_VARN, stress level 2 does all of these all the time */ \ \ diff --git a/src/coreclr/jit/fgstmt.cpp b/src/coreclr/jit/fgstmt.cpp index 57124f6f3d306a..2269e19896308b 100644 --- a/src/coreclr/jit/fgstmt.cpp +++ b/src/coreclr/jit/fgstmt.cpp @@ -18,7 +18,7 @@ bool Compiler::fgBlockContainsStatementBounded(BasicBlock* block, Statement* stmt, bool answerOnBoundExceeded /*= true*/) { - const __int64 maxLinks = 1000000000; + const __int64 maxLinks = 100000000; __int64* numTraversed = &JitTls::GetCompiler()->compNumStatementLinksTraversed; diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 8a7e93de2e7285..b76e407a5c515d 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16201,7 +16201,14 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S // this involves introducing new locals for those values, such that they can // be used in the original statement. // -void Compiler::gtSplitTree( +// Note that this function may introduce new block ops that need to be +// morphed if called after morph. fgMorphStmtBlockOps can be used for this +// purpose. +// +// Returns: +// True if any changes were made; false if nothing needed to be done to split the tree. +// +bool Compiler::gtSplitTree( BasicBlock* block, Statement* stmt, GenTree* splitPoint, Statement** firstNewStmt, GenTree*** splitNodeUse) { class Splitter final : public GenTreeVisitor @@ -16392,7 +16399,7 @@ void Compiler::gtSplitTree( return; } - // Only a handful of nodes can be location, and htey are all unary or nullary. + // Only a handful of nodes can be location, and they are all unary or nullary. assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD)); if ((*use)->OperIsUnary()) { @@ -16417,6 +16424,15 @@ void Compiler::gtSplitTree( } #endif + if ((*use)->OperIs(GT_FIELD_LIST, GT_INIT_VAL)) + { + for (GenTree** operandUse : (*use)->UseEdges()) + { + SplitOutUse(UseInfo{operandUse, *use}, false); + } + return; + } + Statement* stmt = nullptr; if (!(*use)->IsValue() || (*use)->OperIs(GT_ASG) || (user == nullptr) || (user->OperIs(GT_COMMA) && (user->gtGetOp1() == *use))) @@ -16430,14 +16446,6 @@ void Compiler::gtSplitTree( *use = m_compiler->gtNewNothingNode(); MadeChanges = true; } - else if ((*use)->OperIs(GT_FIELD_LIST, GT_INIT_VAL)) - { - for (GenTree** operandUse : (*use)->UseEdges()) - { - SplitOutUse(UseInfo{operandUse, *use}, false); - } - return; - } else { unsigned lclNum = m_compiler->lvaGrabTemp(true DEBUGARG("Spilling to split statement for tree")); @@ -16449,13 +16457,10 @@ void Compiler::gtSplitTree( m_compiler->lvaGetDesc(lclNum)->lvIsMultiRegRet = true; } - if (stmt == nullptr) - { - GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); - stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); - *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); - MadeChanges = true; - } + GenTree* asg = m_compiler->gtNewTempAssign(lclNum, *use); + stmt = m_compiler->fgNewStmtFromTree(asg, m_splitStmt->GetDebugInfo()); + *use = m_compiler->gtNewLclvNode(lclNum, genActualType(*use)); + MadeChanges = true; } if (stmt != nullptr) @@ -16465,8 +16470,6 @@ void Compiler::gtSplitTree( FirstStatement = stmt; } - m_compiler->gtSetStmtInfo(stmt); - m_compiler->fgSetStmtSeq(stmt); m_compiler->fgInsertStmtBefore(m_bb, m_splitStmt, stmt); } } @@ -16480,9 +16483,9 @@ void Compiler::gtSplitTree( if (splitter.MadeChanges) { gtUpdateStmtSideEffects(stmt); - // We may have introduced new LCL_VAR struct uses that need to go through block morphing. - fgMorphBlockStmt(block, stmt DEBUGARG("gtSplitTree")); } + + return splitter.MadeChanges; } //------------------------------------------------------------------------ diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 0a08582a54c0ac..6389a820def55b 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -13484,6 +13484,60 @@ bool Compiler::fgMorphBlockStmt(BasicBlock* block, Statement* stmt DEBUGARG(cons return removedStmt; } +//------------------------------------------------------------------------ +// fgMorphStmtBlockOps: Morph all block ops in the specified statement. +// +// Arguments: +// stmt - the statement +// +void Compiler::fgMorphStmtBlockOps(BasicBlock* block, Statement* stmt) +{ + struct Visitor : GenTreeVisitor + { + enum + { + DoPostOrder = true, + }; + + Visitor(Compiler* comp) : GenTreeVisitor(comp) + { + } + + fgWalkResult PostOrderVisit(GenTree** use, GenTree* user) + { + if ((*use)->OperIsBlkOp()) + { + if ((*use)->OperIs(GT_STORE_DYN_BLK)) + { + *use = m_compiler->fgMorphStoreDynBlock((*use)->AsStoreDynBlk()); + } + else if ((*use)->OperIsInitBlkOp()) + { + *use = m_compiler->fgMorphInitBlock(*use); + } + else + { + *use = m_compiler->fgMorphCopyBlock(*use); + } + } + + return WALK_CONTINUE; + } + }; + + compCurBB = block; + compCurStmt = stmt; + Visitor visitor(this); + visitor.WalkTree(stmt->GetRootNodePointer(), nullptr); + + gtSetStmtInfo(stmt); + + if (fgNodeThreading == NodeThreading::AllTrees) + { + fgSetStmtSeq(stmt); + } +} + /***************************************************************************** * * Morph the statements of the given block. diff --git a/src/coreclr/jit/rationalize.h b/src/coreclr/jit/rationalize.h index e031b0fac356d2..9a86938fd92f50 100644 --- a/src/coreclr/jit/rationalize.h +++ b/src/coreclr/jit/rationalize.h @@ -67,9 +67,6 @@ class Rationalizer final : public Phase inline Rationalizer::Rationalizer(Compiler* _comp) : Phase(_comp, PHASE_RATIONALIZE) { -#ifdef DEBUG - comp->compNumStatementLinksTraversed = 0; -#endif } #endif From c8402e544b9cf7afdc74063ebccb16237f06877e Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 10 Mar 2023 16:53:20 +0100 Subject: [PATCH 13/19] Apply suggestions from code review --- src/coreclr/jit/compiler.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 8370a284f6501f..b9becf58f4de68 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5371,6 +5371,7 @@ PhaseStatus Compiler::StressSplitTree() // void Compiler::SplitTreesRandomly() { +#ifdef DEBUG CLRRandom rng; rng.Init(info.compMethodHash() ^ 0x077cc4d4); @@ -5418,6 +5419,7 @@ void Compiler::SplitTreesRandomly() } } } +#endif } //------------------------------------------------------------------------ From 07cb53afcacec2a62c89eddd11b3edfb14e3bfa1 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 10 Mar 2023 21:26:48 +0100 Subject: [PATCH 14/19] Fix a test --- .../KeepAlive/keepaliveother/keepalivescope.cs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/src/tests/GC/Features/KeepAlive/keepaliveother/keepalivescope.cs b/src/tests/GC/Features/KeepAlive/keepaliveother/keepalivescope.cs index 9d774fbf1d5954..2a5a7599c8a556 100644 --- a/src/tests/GC/Features/KeepAlive/keepaliveother/keepalivescope.cs +++ b/src/tests/GC/Features/KeepAlive/keepaliveother/keepalivescope.cs @@ -4,6 +4,7 @@ // Tests KeepAlive() scopes using System; +using System.Runtime.CompilerServices; public class Test_keepalivescope { @@ -25,11 +26,11 @@ public CreateObj() { obj = new Dummy(); result=false; } - - public void RunTest() { + + [MethodImpl(MethodImplOptions.NoInlining)] + public void RunTestInner() { GC.Collect(); GC.WaitForPendingFinalizers(); - if((Dummy.visited == false)) { // has not visited the Finalize() yet result=true; @@ -38,6 +39,12 @@ public void RunTest() { GC.KeepAlive(obj); // will keep alive 'obj' till this point obj=null; + } + + [MethodImpl(MethodImplOptions.NoInlining)] + public void RunTest() { + RunTestInner(); + GC.Collect(); GC.WaitForPendingFinalizers(); From ba9d3517119eee3bd949778b65bf2b5c4229ef3f Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 10 Mar 2023 22:30:29 +0100 Subject: [PATCH 15/19] Reprocess all new statements to remove commas in them --- src/coreclr/jit/compiler.cpp | 74 ++++++++++++++++++------------------ 1 file changed, 38 insertions(+), 36 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index b9becf58f4de68..68208594f01394 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5429,55 +5429,57 @@ void Compiler::SplitTreesRemoveCommas() { for (BasicBlock* block : Blocks()) { - for (Statement* stmt : block->NonPhiStatements()) + Statement* stmt = block->FirstNonPhiDef(); + while (stmt != nullptr) { - while (true) + Statement* nextStmt = stmt->GetNextStmt(); + for (GenTree* tree : stmt->TreeList()) { - bool again = false; - for (GenTree* tree : stmt->TreeList()) + if (!tree->OperIs(GT_COMMA)) { - if (!tree->OperIs(GT_COMMA)) - { - continue; - } - - // Supporting this weird construct would require additional - // handling, we need to sort of move the comma into to the - // next node in execution order. We don't see this so just - // skip it. - assert(!tree->IsReverseOp()); + continue; + } - JITDUMP("Removing COMMA [%06u]\n", dspTreeID(tree)); - Statement* newStmt; - GenTree** use; - gtSplitTree(block, stmt, tree, &newStmt, &use); - GenTree* op1SideEffects = nullptr; - gtExtractSideEffList(tree->gtGetOp1(), &op1SideEffects); + // Supporting this weird construct would require additional + // handling, we need to sort of move the comma into to the + // next node in execution order. We don't see this so just + // skip it. + assert(!tree->IsReverseOp()); - if (op1SideEffects != nullptr) - { - fgInsertStmtBefore(block, stmt, fgNewStmtFromTree(op1SideEffects)); - } + JITDUMP("Removing COMMA [%06u]\n", dspTreeID(tree)); + Statement* newStmt; + GenTree** use; + gtSplitTree(block, stmt, tree, &newStmt, &use); + GenTree* op1SideEffects = nullptr; + gtExtractSideEffList(tree->gtGetOp1(), &op1SideEffects); - *use = tree->gtGetOp2(); - - while ((newStmt != nullptr) && (newStmt != stmt)) + if (op1SideEffects != nullptr) + { + Statement* op1Stmt = fgNewStmtFromTree(op1SideEffects); + fgInsertStmtBefore(block, stmt, op1Stmt); + if (newStmt == nullptr) { - fgMorphStmtBlockOps(block, newStmt); - newStmt = newStmt->GetNextStmt(); + newStmt = op1Stmt; } - - fgMorphStmtBlockOps(block, stmt); - - again = true; - break; } - if (!again) + *use = tree->gtGetOp2(); + + for (Statement* cur = newStmt; (cur != nullptr) && (cur != stmt); cur = cur->GetNextStmt()) { - break; + fgMorphStmtBlockOps(block, cur); } + + fgMorphStmtBlockOps(block, stmt); + + // Morphing block ops can introduce commas (and the original + // statement can also have more commas left). Proceed from the + // earliest newly introduced statement. + nextStmt = newStmt != nullptr ? newStmt : stmt; + break; } + + stmt = nextStmt; } } From 7a7a683e1f81075ff879fc986daa9611bc4b0e86 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 10 Mar 2023 23:07:21 +0100 Subject: [PATCH 16/19] Leave updating statement flags up to the caller --- src/coreclr/jit/compiler.cpp | 2 ++ src/coreclr/jit/gentree.cpp | 17 +++++++++-------- 2 files changed, 11 insertions(+), 8 deletions(-) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 68208594f01394..880b99647f0b06 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -5410,6 +5410,7 @@ void Compiler::SplitTreesRandomly() } fgMorphStmtBlockOps(block, stmt); + gtUpdateStmtSideEffects(stmt); } break; @@ -5471,6 +5472,7 @@ void Compiler::SplitTreesRemoveCommas() } fgMorphStmtBlockOps(block, stmt); + gtUpdateStmtSideEffects(stmt); // Morphing block ops can introduce commas (and the original // statement can also have more commas left). Proceed from the diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index b76e407a5c515d..58c94dc9c43745 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16201,9 +16201,15 @@ bool Compiler::gtTreeHasSideEffects(GenTree* tree, GenTreeFlags flags /* = GTF_S // this involves introducing new locals for those values, such that they can // be used in the original statement. // -// Note that this function may introduce new block ops that need to be -// morphed if called after morph. fgMorphStmtBlockOps can be used for this -// purpose. +// This function does not update the flags on the original statement as it is +// expected that the caller is going to modify the use further. Thus, the +// caller is responsible for calling gtUpdateStmtSideEffects on the statement, +// though this is only necessary if the function returned true. +// +// The function may introduce new block ops that need to be morphed when used +// after morph. fgMorphStmtBlockOps can be used on the new statements for +// this purpose. Note that this will invalidate the use, so it should be done +// after any further modifications have been made. // // Returns: // True if any changes were made; false if nothing needed to be done to split the tree. @@ -16480,11 +16486,6 @@ bool Compiler::gtSplitTree( *firstNewStmt = splitter.FirstStatement; *splitNodeUse = splitter.SplitNodeUse; - if (splitter.MadeChanges) - { - gtUpdateStmtSideEffects(stmt); - } - return splitter.MadeChanges; } From f052a518dec6ac87b43532ae797a6d6a79a8ba0d Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 11 Mar 2023 00:16:24 +0100 Subject: [PATCH 17/19] Handle locals returned under commas --- src/coreclr/jit/gentree.cpp | 57 ++++++++++++++++++++++++------------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 58c94dc9c43745..544516568752c1 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16263,6 +16263,7 @@ bool Compiler::gtSplitTree( if (*use == m_splitNode) { bool userIsLocation = false; + bool userIsReturned = false; // Split all siblings and ancestor siblings. int i; for (i = 0; i < m_useStack.Height() - 1; i++) @@ -16278,25 +16279,26 @@ bool Compiler::gtSplitTree( // that contains the split node. if (m_useStack.BottomRef(i + 1).User == useInf.User) { - SplitOutUse(useInf, IsLocation(useInf, userIsLocation)); + SplitOutUse(useInf, userIsLocation, userIsReturned); } else { // This is an ancestor of the node we're splitting on. userIsLocation = IsLocation(useInf, userIsLocation); + userIsReturned = IsReturned(useInf, userIsReturned); } } assert(m_useStack.Bottom(i).Use == use); userIsLocation = IsLocation(m_useStack.BottomRef(i), userIsLocation); + userIsReturned = IsReturned(m_useStack.BottomRef(i), userIsReturned); // The remaining nodes should be operands of the split node. for (i++; i < m_useStack.Height(); i++) { const UseInfo& useInf = m_useStack.BottomRef(i); assert(useInf.User == *use); - bool isLocation = IsLocation(useInf, userIsLocation); - SplitOutUse(useInf, isLocation); + SplitOutUse(useInf, userIsLocation, userIsReturned); } SplitNodeUse = use; @@ -16337,7 +16339,25 @@ bool Compiler::gtSplitTree( return false; } - void SplitOutUse(const UseInfo& useInf, bool isLocation) + bool IsReturned(const UseInfo& useInf, bool userIsReturned) + { + if (useInf.User != nullptr) + { + if (useInf.User->OperIs(GT_RETURN)) + { + return true; + } + + if (userIsReturned && useInf.User->OperIs(GT_COMMA) && (useInf.Use == &useInf.User->AsOp()->gtOp2)) + { + return true; + } + } + + return false; + } + + void SplitOutUse(const UseInfo& useInf, bool userIsLocation, bool userIsReturned) { GenTree** use = useInf.Use; GenTree* user = useInf.User; @@ -16368,7 +16388,7 @@ bool Compiler::gtSplitTree( return; } - if (isLocation) + if (IsLocation(useInf, userIsLocation)) { if ((*use)->OperIs(GT_COMMA)) { @@ -16392,15 +16412,17 @@ bool Compiler::gtSplitTree( *use = (*use)->gtGetOp2(); UseInfo use2{use, user}; + // Locations are never returned. + assert(!IsReturned(useInf, userIsReturned)); if ((*use)->IsReverseOp()) { - SplitOutUse(use2, true); - SplitOutUse(use1, false); + SplitOutUse(use2, true, false); + SplitOutUse(use1, false, false); } else { - SplitOutUse(use1, false); - SplitOutUse(use2, true); + SplitOutUse(use1, false, false); + SplitOutUse(use2, true, false); } return; } @@ -16409,13 +16431,10 @@ bool Compiler::gtSplitTree( assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD)); if ((*use)->OperIsUnary()) { - user = *use; - use = &(*use)->AsUnOp()->gtOp1; - } - else - { - return; + SplitOutUse(UseInfo{ &(*use)->AsUnOp()->gtOp1, user }, false, false); } + + return; } #ifndef TARGET_64BIT @@ -16425,8 +16444,8 @@ bool Compiler::gtSplitTree( if ((user != nullptr) && user->OperIs(GT_MUL) && user->Is64RsltMul()) { assert((*use)->OperIs(GT_CAST)); - user = *use; - use = &(*use)->AsCast()->gtOp1; + SplitOutUse(UseInfo{ &(*use)->AsCast()->gtOp1, *use }, false, false); + return; } #endif @@ -16434,7 +16453,7 @@ bool Compiler::gtSplitTree( { for (GenTree** operandUse : (*use)->UseEdges()) { - SplitOutUse(UseInfo{operandUse, *use}, false); + SplitOutUse(UseInfo{operandUse, *use}, false, false); } return; } @@ -16458,7 +16477,7 @@ bool Compiler::gtSplitTree( if (varTypeIsStruct(*use) && ((*use)->IsMultiRegNode() || - ((user != nullptr) && user->OperIs(GT_RETURN) && m_compiler->compMethodReturnsMultiRegRetType()))) + (IsReturned(useInf, userIsReturned) && m_compiler->compMethodReturnsMultiRegRetType()))) { m_compiler->lvaGetDesc(lclNum)->lvIsMultiRegRet = true; } From dc6e220fd924679860587de8113223eb1a30798c Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 11 Mar 2023 00:17:30 +0100 Subject: [PATCH 18/19] Run jit-format --- src/coreclr/jit/gentree.cpp | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 544516568752c1..a2402ae7e17674 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16431,7 +16431,7 @@ bool Compiler::gtSplitTree( assert((*use)->OperIs(GT_IND, GT_OBJ, GT_BLK, GT_LCL_VAR, GT_LCL_FLD)); if ((*use)->OperIsUnary()) { - SplitOutUse(UseInfo{ &(*use)->AsUnOp()->gtOp1, user }, false, false); + SplitOutUse(UseInfo{&(*use)->AsUnOp()->gtOp1, user}, false, false); } return; @@ -16444,7 +16444,7 @@ bool Compiler::gtSplitTree( if ((user != nullptr) && user->OperIs(GT_MUL) && user->Is64RsltMul()) { assert((*use)->OperIs(GT_CAST)); - SplitOutUse(UseInfo{ &(*use)->AsCast()->gtOp1, *use }, false, false); + SplitOutUse(UseInfo{&(*use)->AsCast()->gtOp1, *use}, false, false); return; } #endif From ef4943bf72a9c19c3dadfe6546e24b7487921d42 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Sat, 11 Mar 2023 13:32:40 +0100 Subject: [PATCH 19/19] Put new phase under ifdef --- src/coreclr/jit/compiler.cpp | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/coreclr/jit/compiler.cpp b/src/coreclr/jit/compiler.cpp index 600d44e7f9d0ed..13177d52753961 100644 --- a/src/coreclr/jit/compiler.cpp +++ b/src/coreclr/jit/compiler.cpp @@ -4996,7 +4996,9 @@ void Compiler::compCompile(void** methodCodePtr, uint32_t* methodCodeSize, JitFl fgDomsComputed = false; optLoopTableValid = false; +#ifdef DEBUG DoPhase(this, PHASE_STRESS_SPLIT_TREE, &Compiler::StressSplitTree); +#endif // Insert GC Polls DoPhase(this, PHASE_INSERT_GC_POLLS, &Compiler::fgInsertGCPolls);