JIT: Enable inlining for late devirtualization #110827
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -204,7 +204,9 @@ bool Compiler::TypeInstantiationComplexityExceeds(CORINFO_CLASS_HANDLE handle, i | |
|
|
||
| class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<SubstitutePlaceholdersAndDevirtualizeWalker> | ||
| { | ||
| bool m_madeChanges = false; | ||
| Statement* m_curStmt = nullptr; | ||
| Statement* m_firstNewStmt = nullptr; | ||
|
|
||
| public: | ||
| enum | ||
|
|
@@ -219,11 +221,29 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi | |
| { | ||
| } | ||
|
|
||
| bool MadeChanges() const | ||
| { | ||
| return m_madeChanges; | ||
| } | ||
|
|
||
| // ------------------------------------------------------------------------ | ||
| // WalkStatement: Walk the tree of a statement, and return the first newly | ||
| // added statement if any, otherwise return the original statement. | ||
| // | ||
| // Arguments: | ||
| // stmt - the statement to walk. | ||
| // | ||
| // Return Value: | ||
| // The first newly added statement if any, or the original statement. | ||
| // | ||
| Statement* WalkStatement(Statement* stmt) | ||
| { | ||
| m_curStmt = stmt; | ||
| m_firstNewStmt = nullptr; | ||
| WalkTree(m_curStmt->GetRootNodePointer(), nullptr); | ||
| return m_firstNewStmt == nullptr ? m_curStmt : m_firstNewStmt; | ||
| } | ||
|
|
||
| fgWalkResult PreOrderVisit(GenTree** use, GenTree* user) | ||
| { | ||
| GenTree* tree = *use; | ||
|
|
@@ -586,8 +606,59 @@ class SubstitutePlaceholdersAndDevirtualizeWalker : public GenTreeVisitor<Substi | |
| } | ||
|
|
||
| CORINFO_CONTEXT_HANDLE contextInput = context; | ||
| context = nullptr; | ||
| m_compiler->impDevirtualizeCall(call, nullptr, &method, &methodFlags, &contextInput, &context, | ||
| isLateDevirtualization, explicitTailCall); | ||
|
|
||
| if (!call->IsVirtual()) | ||
| { | ||
| assert(context != nullptr); | ||
| CORINFO_CALL_INFO callInfo = {}; | ||
| callInfo.hMethod = method; | ||
| callInfo.methodFlags = methodFlags; | ||
| m_compiler->impMarkInlineCandidate(call, context, false, &callInfo); | ||
|
|
||
| if (call->IsInlineCandidate()) | ||
| { | ||
| Statement* newStmt = nullptr; | ||
| GenTree** callUse = nullptr; | ||
| if (m_compiler->gtSplitTree(m_compiler->compCurBB, m_curStmt, call, &newStmt, &callUse, true)) | ||
| { | ||
| if (m_firstNewStmt == nullptr) | ||
| { | ||
| m_firstNewStmt = newStmt; | ||
| } | ||
| } | ||
|
|
||
| // If the call is the root expression in a statement, and it returns void, | ||
| // we can inline it directly without creating a RET_EXPR. | ||
| if (parent != nullptr || call->gtReturnType != TYP_VOID) | ||
hez2010 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| { | ||
| Statement* stmt = m_compiler->gtNewStmt(call); | ||
| m_compiler->fgInsertStmtBefore(m_compiler->compCurBB, m_curStmt, stmt); | ||
| if (m_firstNewStmt == nullptr) | ||
| { | ||
| m_firstNewStmt = stmt; | ||
| } | ||
|
|
||
| GenTreeRetExpr* retExpr = | ||
| m_compiler->gtNewInlineCandidateReturnExpr(call->AsCall(), | ||
| genActualType(call->TypeGet())); | ||
| call->GetSingleInlineCandidateInfo()->retExpr = retExpr; | ||
|
|
||
| JITDUMP("Creating new RET_EXPR for [%06u]:\n", call->gtTreeID); | ||
| DISPTREE(retExpr); | ||
|
|
||
| *pTree = retExpr; | ||
| } | ||
|
|
||
| call->GetSingleInlineCandidateInfo()->exactContextHandle = context; | ||
| INDEBUG(call->GetSingleInlineCandidateInfo()->inlinersContext = call->gtInlineContext); | ||
|
There was a problem hiding this comment. @hez2010 I think this Debug logic is causing Debug and Release JITs to report differing amounts of metadata to SuperPMI. runtime-coreclr superpmi-asmdiffs-checked-release broke around the time this PR was merged, and I'm not getting any diffs locally when using builds from the commit before this one. cc @AndyAyersMS @jakobbotsch. I'm not sure what the ideal fix is here, since There was a problem hiding this comment. I think we may need to move There was a problem hiding this comment. Can the information be put into There was a problem hiding this comment. We have avoided trying to add more fields to GenTreeCall that are only needed for some small subset of all calls. What @jakobbotsch suggests makes sense to me. There was a problem hiding this comment. Okay I revised this issue and found that the issue is that the debug logic should really be INDEBUG(call->gtInlineContext = call->GetSingleInlineCandidateInfo()->inlinersContext);instead. We set a new The fix is now live: #112396 |
||
|
|
||
| JITDUMP("New inline candidate due to late devirtualization:\n"); | ||
| DISPTREE(call); | ||
| } | ||
| } | ||
| m_madeChanges = true; | ||
| } | ||
| } | ||
|
|
@@ -730,17 +801,10 @@ PhaseStatus Compiler::fgInline() | |
| do | ||
| { | ||
| // Make the current basic block address available globally | ||
| compCurBB = block; | ||
| Statement* stmt = block->firstStmt(); | ||
| while (stmt != nullptr) | ||
| { | ||
| // See if we need to replace some return value place holders. | ||
| // Also, see if this replacement enables further devirtualization. | ||
| // | ||
|
|
@@ -755,7 +819,7 @@ PhaseStatus Compiler::fgInline() | |
| // possible further optimization, as the (now complete) GT_RET_EXPR | ||
| // replacement may have enabled optimizations by providing more | ||
| // specific types for trees or variables. | ||
| stmt = walker.WalkStatement(stmt); | ||
|
|
||
| GenTree* expr = stmt->GetRootNode(); | ||
|
|
||
|
|
@@ -805,6 +869,13 @@ PhaseStatus Compiler::fgInline() | |
| madeChanges = true; | ||
| stmt->SetRootNode(expr->AsOp()->gtOp1); | ||
| } | ||
|
|
||
| #if defined(DEBUG) | ||
| // In debug builds we want the inline tree to show all failed | ||
| // inlines. | ||
| fgWalkTreePre(stmt->GetRootNodePointer(), fgFindNonInlineCandidate, stmt); | ||
| #endif | ||
| stmt = stmt->GetNextStmt(); | ||
| } | ||
|
|
||
| block = block->Next(); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Strictly speaking this split is unnecessarily conservative: it will introduce temps for the arguments of the call, which can bypass optimizations the inliner can make depending on the specific shape of the argument. Fixing that requires enhancing
gtSplitTreea bit (see e.g. my version in this branch), however, even with that done you will run into issues around incorrect treatment of the retbuffer in the inliner. So I wouldn't suggest trying to do anything about it at this point, but it is a potential future improvement.