From 3c494498b106ec7878a506d3fd3434ca6627e947 Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 16 Dec 2022 13:05:08 +0100 Subject: [PATCH 1/2] JIT: Clean up some old style walks Extracted from early liveness PR. Also small visitor change. --- src/coreclr/jit/compiler.h | 14 ++++-------- src/coreclr/jit/flowgraph.cpp | 24 -------------------- src/coreclr/jit/gentree.cpp | 41 +++++++++++++++++++++++++++++++++++ src/coreclr/jit/morph.cpp | 8 ++----- 4 files changed, 47 insertions(+), 40 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index d19e1ee8b92fa..44482a3effb0f 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5450,9 +5450,6 @@ class Compiler void* pCallBackData = nullptr, bool computeStack = false); - static fgWalkResult fgChkLocAllocCB(GenTree** pTree, Compiler::fgWalkData* data); - static fgWalkResult fgChkQmarkCB(GenTree** pTree, Compiler::fgWalkData* data); - /************************************************************************** * PROTECTED *************************************************************************/ @@ -5931,6 +5928,7 @@ class Compiler bool gtIsTypeHandleToRuntimeTypeHandleHelper(GenTreeCall* call, CorInfoHelpFunc* pHelper = nullptr); bool gtIsActiveCSE_Candidate(GenTree* tree); + bool gtTreeContainsOper(GenTree* tree, genTreeOps op); ExceptionSetFlags gtCollectExceptions(GenTree* tree); bool fgIsBigOffset(size_t offset); @@ -10932,21 +10930,17 @@ class GenTreeVisitor { GenTreeStoreDynBlk* const dynBlock = node->AsStoreDynBlk(); - GenTree** op1Use = &dynBlock->gtOp1; - GenTree** op2Use = &dynBlock->gtOp2; - GenTree** op3Use = &dynBlock->gtDynamicSize; - - result = WalkTree(op1Use, dynBlock); + result = WalkTree(&dynBlock->gtOp1, dynBlock); if (result == fgWalkResult::WALK_ABORT) { return result; } - result = WalkTree(op2Use, dynBlock); + result = WalkTree(&dynBlock->gtOp2, dynBlock); if (result == fgWalkResult::WALK_ABORT) { return result; } - result = WalkTree(op3Use, dynBlock); + result = WalkTree(&dynBlock->gtDynamicSize, dynBlock); if (result == fgWalkResult::WALK_ABORT) { return result; diff --git a/src/coreclr/jit/flowgraph.cpp b/src/coreclr/jit/flowgraph.cpp index 5c02a6acb3bab..794f4c8f7aff0 100644 --- a/src/coreclr/jit/flowgraph.cpp +++ b/src/coreclr/jit/flowgraph.cpp @@ -4230,30 +4230,6 @@ void Compiler::fgSetBlockOrder(BasicBlock* block) return firstNode; } -/*static*/ Compiler::fgWalkResult Compiler::fgChkLocAllocCB(GenTree** pTree, fgWalkData* data) -{ - GenTree* tree = *pTree; - - if (tree->gtOper == GT_LCLHEAP) - { - return Compiler::WALK_ABORT; - } - - return Compiler::WALK_CONTINUE; -} - -/*static*/ Compiler::fgWalkResult Compiler::fgChkQmarkCB(GenTree** pTree, fgWalkData* data) -{ - GenTree* tree = *pTree; - - if (tree->gtOper == GT_QMARK) - { - return Compiler::WALK_ABORT; - } - - return Compiler::WALK_CONTINUE; -} - void Compiler::fgLclFldAssign(unsigned lclNum) { assert(varTypeIsStruct(lvaTable[lclNum].lvType)); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 1999b79c70e52..50a5615b8afaa 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -16465,6 +16465,47 @@ bool Compiler::gtIsActiveCSE_Candidate(GenTree* tree) return (optValnumCSE_phase && IS_CSE_INDEX(tree->gtCSEnum)); } +//------------------------------------------------------------------------ +// gtTreeContainsOper -- check if the tree contains any subtree with the specified oper. +// +// Arguments: +// tree - tree to examine +// oper - oper to check for +// +// Return Value: +// True if any subtree has the specified oper; otherwise false. +// +bool Compiler::gtTreeContainsOper(GenTree* tree, genTreeOps oper) +{ + class Visitor final : public GenTreeVisitor + { + genTreeOps m_oper; + + public: + Visitor(Compiler* comp, genTreeOps oper) : GenTreeVisitor(comp), m_oper(oper) + { + } + + enum + { + DoPreOrder = true, + }; + + fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) + { + if ((*use)->OperIs(m_oper)) + { + return WALK_ABORT; + } + + return WALK_CONTINUE; + } + }; + + Visitor visitor(this, oper); + return visitor.WalkTree(&tree, nullptr) == WALK_ABORT; +} + //------------------------------------------------------------------------ // gtCollectExceptions: walk a tree collecting a bit set of exceptions the tree // may throw. diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index bcfe29063d15b..7580ce1603a7d 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1207,9 +1207,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) { assert(comp->compLocallocUsed); - // Returns WALK_ABORT if a GT_LCLHEAP node is encountered in the argx tree - // - if (comp->fgWalkTreePre(&argx, Compiler::fgChkLocAllocCB) == Compiler::WALK_ABORT) + if (comp->gtTreeContainsOper(argx, GT_LCLHEAP)) { SetNeedsTemp(&arg); continue; @@ -1219,9 +1217,7 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } if (hasStructRegArgWeCareAbout) { - // Returns true if a GT_QMARK node is encountered in the argx tree - // - if (comp->fgWalkTreePre(&argx, Compiler::fgChkQmarkCB) == Compiler::WALK_ABORT) + if (comp->gtTreeContainsOper(argx, GT_QMARK)) { SetNeedsTemp(&arg); continue; From da4bc9187e19c5bd4019ba3ab38b9c27c6e6fdab Mon Sep 17 00:00:00 2001 From: Jakob Botsch Nielsen Date: Fri, 16 Dec 2022 17:56:17 +0100 Subject: [PATCH 2/2] Remove dead code, more cleanup --- src/coreclr/jit/compiler.h | 4 +- src/coreclr/jit/gentree.cpp | 7 +-- src/coreclr/jit/morph.cpp | 107 +++++++++++++----------------------- 3 files changed, 40 insertions(+), 78 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 44482a3effb0f..a7b46b92e23fe 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -4622,10 +4622,8 @@ class Compiler void fgSetOptions(); #ifdef DEBUG - static fgWalkPreFn fgAssertNoQmark; void fgPreExpandQmarkChecks(GenTree* expr); - void fgPostExpandQmarkChecks(); - static void fgCheckQmarkAllowedForm(GenTree* tree); + void fgPostExpandQmarkChecks(); #endif IL_OFFSET fgFindBlockILOffset(BasicBlock* block); diff --git a/src/coreclr/jit/gentree.cpp b/src/coreclr/jit/gentree.cpp index 50a5615b8afaa..aa939188c49f9 100644 --- a/src/coreclr/jit/gentree.cpp +++ b/src/coreclr/jit/gentree.cpp @@ -6825,12 +6825,7 @@ GenTreeQmark* Compiler::gtNewQmarkNode(var_types type, GenTree* cond, GenTreeCol { compQmarkUsed = true; GenTreeQmark* result = new (this, GT_QMARK) GenTreeQmark(type, cond, colon); -#ifdef DEBUG - if (compQmarkRationalized) - { - fgCheckQmarkAllowedForm(result); - } -#endif + assert(!compQmarkRationalized && "QMARKs are illegal to create after QMARK-rationalization"); return result; } diff --git a/src/coreclr/jit/morph.cpp b/src/coreclr/jit/morph.cpp index 7580ce1603a7d..14403a8c0f796 100644 --- a/src/coreclr/jit/morph.cpp +++ b/src/coreclr/jit/morph.cpp @@ -1149,19 +1149,13 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) #endif // FEATURE_MULTIREG_ARGS } - // We only care because we can't spill structs and qmarks involve a lot of spilling, but - // if we don't have qmarks, then it doesn't matter. - // So check for Qmark's globally once here, instead of inside the loop. - // - const bool hasStructRegArgWeCareAbout = (hasStructRegArg && comp->compQmarkUsed); - #if FEATURE_FIXED_OUT_ARGS // For Arm/x64 we only care because we can't reorder a register // argument that uses GT_LCLHEAP. This is an optimization to // save a check inside the below loop. // - const bool hasStackArgsWeCareAbout = (m_hasStackArgs && comp->compLocallocUsed); + const bool hasStackArgsWeCareAbout = m_hasStackArgs && comp->compLocallocUsed; #else @@ -1176,11 +1170,12 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) // a GT_IND with GTF_IND_RNGCHK (only on x86) or // a GT_LCLHEAP node that allocates stuff on the stack // - if (hasStackArgsWeCareAbout || hasStructRegArgWeCareAbout) + if (hasStackArgsWeCareAbout) { for (CallArg& arg : EarlyArgs()) { GenTree* argx = arg.GetEarlyNode(); + assert(!comp->gtTreeContainsOper(argx, GT_QMARK)); // Examine the register args that are currently not marked needTmp // @@ -1215,14 +1210,6 @@ void CallArgs::ArgsComplete(Compiler* comp, GenTreeCall* call) } #endif } - if (hasStructRegArgWeCareAbout) - { - if (comp->gtTreeContainsOper(argx, GT_QMARK)) - { - SetNeedsTemp(&arg); - continue; - } - } } } } @@ -14188,39 +14175,24 @@ GenTree* Compiler::fgInitThisClass() } #ifdef DEBUG -/***************************************************************************** - * - * Tree walk callback to make sure no GT_QMARK nodes are present in the tree, - * except for the allowed ? 1 : 0; pattern. - */ -Compiler::fgWalkResult Compiler::fgAssertNoQmark(GenTree** tree, fgWalkData* data) -{ - if ((*tree)->OperGet() == GT_QMARK) - { - fgCheckQmarkAllowedForm(*tree); - } - return WALK_CONTINUE; -} -void Compiler::fgCheckQmarkAllowedForm(GenTree* tree) -{ - assert(tree->OperGet() == GT_QMARK); - assert(!"Qmarks beyond morph disallowed."); -} - -/***************************************************************************** - * - * Verify that the importer has created GT_QMARK nodes in a way we can - * process them. The following is allowed: - * - * 1. A top level qmark. Top level qmark is of the form: - * a) (bool) ? (void) : (void) OR - * b) V0N = (bool) ? (type) : (type) - * - * 2. Recursion is allowed at the top level, i.e., a GT_QMARK can be a child - * of either op1 of colon or op2 of colon but not a child of any other - * operator. - */ +//------------------------------------------------------------------------ +// fgPreExpandQmarkChecks: Verify that the importer has created GT_QMARK nodes +// in a way we can process them. The following +// +// Returns: +// Suitable phase status. +// +// Remarks: +// The following is allowed: +// 1. A top level qmark. Top level qmark is of the form: +// a) (bool) ? (void) : (void) OR +// b) V0N = (bool) ? (type) : (type) +// +// 2. Recursion is allowed at the top level, i.e., a GT_QMARK can be a child +// of either op1 of colon or op2 of colon but not a child of any other +// operator. +// void Compiler::fgPreExpandQmarkChecks(GenTree* expr) { GenTree* topQmark = fgGetTopLevelQmark(expr); @@ -14229,18 +14201,34 @@ void Compiler::fgPreExpandQmarkChecks(GenTree* expr) // there are no qmarks within it. if (topQmark == nullptr) { - fgWalkTreePre(&expr, Compiler::fgAssertNoQmark, nullptr); + assert(!gtTreeContainsOper(expr, GT_QMARK) && "Illegal QMARK"); } else { // We could probably expand the cond node also, but don't think the extra effort is necessary, // so let's just assert the cond node of a top level qmark doesn't have further top level qmarks. - fgWalkTreePre(&topQmark->AsOp()->gtOp1, Compiler::fgAssertNoQmark, nullptr); + assert(!gtTreeContainsOper(topQmark->AsOp()->gtOp1, GT_QMARK) && "Illegal QMARK"); fgPreExpandQmarkChecks(topQmark->AsOp()->gtOp2->AsOp()->gtOp1); fgPreExpandQmarkChecks(topQmark->AsOp()->gtOp2->AsOp()->gtOp2); } } + +//------------------------------------------------------------------------ +// fgPostExpandQmarkChecks: Make sure we don't have any more GT_QMARK nodes. +// +void Compiler::fgPostExpandQmarkChecks() +{ + for (BasicBlock* const block : Blocks()) + { + for (Statement* const stmt : block->Statements()) + { + GenTree* expr = stmt->GetRootNode(); + assert(!gtTreeContainsOper(expr, GT_QMARK) && "QMARKs are disallowed beyond morph"); + } + } +} + #endif // DEBUG /***************************************************************************** @@ -14701,25 +14689,6 @@ void Compiler::fgExpandQmarkNodes() compQmarkRationalized = true; } -#ifdef DEBUG -/***************************************************************************** - * - * Make sure we don't have any more GT_QMARK nodes. - * - */ -void Compiler::fgPostExpandQmarkChecks() -{ - for (BasicBlock* const block : Blocks()) - { - for (Statement* const stmt : block->Statements()) - { - GenTree* expr = stmt->GetRootNode(); - fgWalkTreePre(&expr, Compiler::fgAssertNoQmark, nullptr); - } - } -} -#endif - //------------------------------------------------------------------------ // fgPromoteStructs: promote structs to collections of per-field locals //