From 35d7df80c35900d1b1c93f35125c74bad835c2ca Mon Sep 17 00:00:00 2001 From: EgorBo Date: Sat, 22 Apr 2023 19:29:29 +0200 Subject: [PATCH 1/3] Don't re-scan previously visited blocks in helperexpansion.cpp --- src/coreclr/jit/compiler.h | 12 +++++----- src/coreclr/jit/helperexpansion.cpp | 35 +++++++++++++++++------------ 2 files changed, 27 insertions(+), 20 deletions(-) diff --git a/src/coreclr/jit/compiler.h b/src/coreclr/jit/compiler.h index 426e790f712ba..fafea6f188150 100644 --- a/src/coreclr/jit/compiler.h +++ b/src/coreclr/jit/compiler.h @@ -5301,20 +5301,20 @@ class Compiler void SplitTreesRandomly(); void SplitTreesRemoveCommas(); - template + template PhaseStatus fgExpandHelper(bool skipRarelyRunBlocks); - template - bool fgExpandHelperForBlock(BasicBlock* block); + template + bool fgExpandHelperForBlock(BasicBlock** pBlock); PhaseStatus fgExpandRuntimeLookups(); - bool fgExpandRuntimeLookupsForCall(BasicBlock* block, Statement* stmt, GenTreeCall* call); + bool fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call); PhaseStatus fgExpandThreadLocalAccess(); - bool fgExpandThreadLocalAccessForCall(BasicBlock* block, Statement* stmt, GenTreeCall* call); + bool fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call); PhaseStatus fgExpandStaticInit(); - bool fgExpandStaticInitForCall(BasicBlock* block, Statement* stmt, GenTreeCall* call); + bool fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call); PhaseStatus fgInsertGCPolls(); BasicBlock* fgCreateGCPoll(GCPollType pollType, BasicBlock* block); diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index 2f03327a8eeb3..0901159bb1da0 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -93,8 +93,9 @@ PhaseStatus Compiler::fgExpandRuntimeLookups() return fgExpandHelper<&Compiler::fgExpandRuntimeLookupsForCall>(false); } -bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock* block, Statement* stmt, GenTreeCall* call) +bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call) { + BasicBlock* block = *pBlock; assert(call->IsHelperCall()); if (!call->IsExpRuntimeLookup()) @@ -150,6 +151,7 @@ bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock* block, Statement* stmt, GenTree** callUse = nullptr; Statement* newFirstStmt = nullptr; block = fgSplitBlockBeforeTree(block, stmt, call, &newFirstStmt, &callUse); + *pBlock = block; assert(prevBb != nullptr && block != nullptr); // Block ops inserted by the split need to be morphed here since we are after morph. @@ -435,9 +437,9 @@ PhaseStatus Compiler::fgExpandThreadLocalAccess() // that access fields marked with [ThreadLocal]. // // Arguments: -// block - Block containing the helper call to expand -// stmt - Statement containing the helper call -// call - The helper call +// pBlock - Block containing the helper call to expand (may be updated in case of block splitting) +// stmt - Statement containing the helper call +// call - The helper call // // // Returns: @@ -452,8 +454,9 @@ PhaseStatus Compiler::fgExpandThreadLocalAccess() // If the entry is not present, the helper is called, which would make an entry of current static block // in the cache. // -bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock* block, Statement* stmt, GenTreeCall* call) +bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call) { + BasicBlock* block = *pBlock; assert(call->IsHelperCall()); if (!call->IsExpTLSFieldAccess()) { @@ -491,6 +494,7 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock* block, Statement* st Statement* newFirstStmt = nullptr; DebugInfo debugInfo = stmt->GetDebugInfo(); block = fgSplitBlockBeforeTree(block, stmt, call, &newFirstStmt, &callUse); + *pBlock = block; assert(prevBb != nullptr && block != nullptr); // Block ops inserted by the split need to be morphed here since we are after morph. @@ -684,7 +688,7 @@ bool Compiler::fgExpandThreadLocalAccessForCall(BasicBlock* block, Statement* st // Returns: // true if there was any helper that was expanded. // -template +template PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) { PhaseStatus result = PhaseStatus::MODIFIED_NOTHING; @@ -697,7 +701,7 @@ PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) } // Expand and visit the last block again to find more candidates - while (fgExpandHelperForBlock(block)) + while (fgExpandHelperForBlock(&block)) { result = PhaseStatus::MODIFIED_EVERYTHING; } @@ -717,16 +721,17 @@ PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) // invoke `fgExpand` if any of the tree node was a helper call. // // Arguments: -// block - block to scan for static initializations +// pBlock - block to scan for static initializations +// (may be updated in case of block splitting) // fgExpand - function that expands the helper call // // Returns: // true if a helper was expanded // -template -bool Compiler::fgExpandHelperForBlock(BasicBlock* block) +template +bool Compiler::fgExpandHelperForBlock(BasicBlock** pBlock) { - for (Statement* const stmt : block->NonPhiStatements()) + for (Statement* const stmt : (*pBlock)->NonPhiStatements()) { if ((stmt->GetRootNode()->gtFlags & GTF_CALL) == 0) { @@ -741,7 +746,7 @@ bool Compiler::fgExpandHelperForBlock(BasicBlock* block) continue; } - if ((this->*ExpansionFunction)(block, stmt, tree->AsCall())) + if ((this->*ExpansionFunction)(pBlock, stmt, tree->AsCall())) { return true; } @@ -798,15 +803,16 @@ PhaseStatus Compiler::fgExpandStaticInit() // Also, see fgExpandStaticInit's comments. // // Arguments: -// block - call's block +// pBlock - call's block (may be updated in case of block splitting) // stmt - call's statement // call - call that represents a static initialization // // Returns: // true if a static initialization was expanded // -bool Compiler::fgExpandStaticInitForCall(BasicBlock* block, Statement* stmt, GenTreeCall* call) +bool Compiler::fgExpandStaticInitForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call) { + BasicBlock* block = *pBlock; assert(call->IsHelperCall()); bool isGc = false; @@ -850,6 +856,7 @@ bool Compiler::fgExpandStaticInitForCall(BasicBlock* block, Statement* stmt, Gen GenTree** callUse = nullptr; Statement* newFirstStmt = nullptr; block = fgSplitBlockBeforeTree(block, stmt, call, &newFirstStmt, &callUse); + *pBlock = block; assert(prevBb != nullptr && block != nullptr); // Block ops inserted by the split need to be morphed here since we are after morph. From 19c4822b719486201ea66acd0232d0749ce210e1 Mon Sep 17 00:00:00 2001 From: EgorBo Date: Wed, 26 Apr 2023 01:40:20 +0200 Subject: [PATCH 2/3] Address feedback --- src/coreclr/jit/helperexpansion.cpp | 38 ++++++++++++++++++++++++----- 1 file changed, 32 insertions(+), 6 deletions(-) diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index f534b718b5dbd..07d6b6d3f3f71 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -93,6 +93,25 @@ PhaseStatus Compiler::fgExpandRuntimeLookups() return fgExpandHelper<&Compiler::fgExpandRuntimeLookupsForCall>(false); } +//------------------------------------------------------------------------------ +// fgExpandRuntimeLookupsForCall : partially expand runtime lookups helper calls +// to add a nullcheck [+ size check] and a fast path +// +// Arguments: +// pBlock - Block containing the helper call to expand. If expansion is performed, +// this is updated to the new block that was an outcome of block splitting. +// stmt - Statement containing the helper call +// call - The helper call +// +// Returns: +// true if a runtime lookup was found and expanded. +// +// Notes: +// The runtime lookup itself is needed to access a handle in code shared between +// generic instantiations. The lookup depends on the typeContext which is only available at +// runtime, and not at compile - time. See ASCII block diagrams in comments below for +// better understanding how this phase expands runtime lookups. +// bool Compiler::fgExpandRuntimeLookupsForCall(BasicBlock** pBlock, Statement* stmt, GenTreeCall* call) { BasicBlock* block = *pBlock; @@ -435,7 +454,8 @@ PhaseStatus Compiler::fgExpandThreadLocalAccess() // that access fields marked with [ThreadLocal]. // // Arguments: -// pBlock - Block containing the helper call to expand (may be updated in case of block splitting) +// pBlock - Block containing the helper call to expand. If expansion is performed, +// this is updated to the new block that was an outcome of block splitting. // stmt - Statement containing the helper call // call - The helper call // @@ -699,9 +719,14 @@ PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) } // Expand and visit the last block again to find more candidates + INDEBUG(BasicBlock* origBlock = block); while (fgExpandHelperForBlock(&block)) { result = PhaseStatus::MODIFIED_EVERYTHING; +#ifdef DEBUG + assert(origBlock != block); + origBlock = block; +#endif } } @@ -719,8 +744,8 @@ PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) // invoke `fgExpand` if any of the tree node was a helper call. // // Arguments: -// pBlock - block to scan for static initializations -// (may be updated in case of block splitting) +// pBlock - Block containing the helper call to expand. If expansion is performed, +// this is updated to the new block that was an outcome of block splitting. // fgExpand - function that expands the helper call // // Returns: @@ -801,9 +826,10 @@ PhaseStatus Compiler::fgExpandStaticInit() // Also, see fgExpandStaticInit's comments. // // Arguments: -// pBlock - call's block (may be updated in case of block splitting) -// stmt - call's statement -// call - call that represents a static initialization +// pBlock - Block containing the helper call to expand. If expansion is performed, +// this is updated to the new block that was an outcome of block splitting. +// stmt - Statement containing the helper call +// call - The helper call // // Returns: // true if a static initialization was expanded From 46e8ba1b616b8a0167f54da4147ede72fd654821 Mon Sep 17 00:00:00 2001 From: Egor Bogatov Date: Wed, 26 Apr 2023 02:55:29 +0200 Subject: [PATCH 3/3] Update helperexpansion.cpp --- src/coreclr/jit/helperexpansion.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/coreclr/jit/helperexpansion.cpp b/src/coreclr/jit/helperexpansion.cpp index 07d6b6d3f3f71..e614e781eb388 100644 --- a/src/coreclr/jit/helperexpansion.cpp +++ b/src/coreclr/jit/helperexpansion.cpp @@ -725,7 +725,7 @@ PhaseStatus Compiler::fgExpandHelper(bool skipRarelyRunBlocks) result = PhaseStatus::MODIFIED_EVERYTHING; #ifdef DEBUG assert(origBlock != block); - origBlock = block; + origBlock = block; #endif } }