JIT: remove single-def restriction from escape analysis type refinement #113808
Conversation
Instead, model all assignments to unescaped locals in the connection graph, and deduce the new type via graph analysis. Update the INDEX_ADDR expansion to handle cases where the array is stack allocated (interior pointers are now TYP_I_IMPL, not TYP_BYREF).
There was a problem hiding this comment.
Copilot wasn't able to review any files in this pull request.
Files not reviewed (5)
- src/coreclr/jit/gentree.cpp: Language not supported
- src/coreclr/jit/gentree.h: Language not supported
- src/coreclr/jit/morph.cpp: Language not supported
- src/coreclr/jit/objectalloc.cpp: Language not supported
- src/coreclr/jit/objectalloc.h: Language not supported
ghost
added
the
area-CodeGen-coreclr
|
@dotnet/jit-contrib PTAL This is mainly an enabling change for field-wise escape analysis but has some diffs of its own. Code diffs best viewed with whitespace suppressed, as I refactored a large method and indentation levels changed. |
src/coreclr/jit/objectalloc.cpp
Outdated
| LclVarDsc* lclVarDsc = comp->lvaGetDesc(lclNum); | ||
|
|
||
| if ((lclVarDsc->lvSingleDef == 1) && !comp->opts.IsOSR()) | ||
| if (!comp->opts.IsOSR()) |
There was a problem hiding this comment.
This is the heart of the change; the rest is there to ensure the deduction we make here is sound.
We now need to know "everything" that can be assigned to each local.
| if (tree->OperIsLocalStore()) | ||
| { | ||
| GenTreeLclVarCommon* const lclTree = tree->AsLclVarCommon(); | ||
| unsigned const lclNum = lclTree->GetLclNum(); | ||
| if (m_allocator->IsTrackedLocal(lclNum) && !m_allocator->CanLclVarEscape(lclNum)) | ||
| { | ||
| if ((lclTree->gtFlags & GTF_VAR_CONNECTED) == 0) | ||
| { | ||
| // This store was not modelled in the connection graph. | ||
| // | ||
| // If the stored value was was not a stack-viable allocation or null, | ||
| // add an edge to unknown source. This will ensure this local does | ||
| // not get retyped as TYP_I_IMPL. | ||
| // | ||
| GenTree* const data = lclTree->Data(); | ||
| if (!data->IsIntegralConst(0) && (m_allocator->AllocationKind(data) == OAT_NONE)) | ||
| { | ||
| // Add a connection to the unknown source. | ||
| // | ||
| JITDUMP("V%02u value unknown at [%06u]\n", lclNum, m_compiler->dspTreeID(tree)); | ||
| m_allocator->AddConnGraphEdge(lclNum, m_allocator->m_unknownSourceLocalNum); | ||
| } | ||
| } | ||
| } | ||
| tree->gtFlags &= ~GTF_VAR_CONNECTED; | ||
| } |
There was a problem hiding this comment.
Just curious -- why is some handling needed here, and some handling in the other place? Why isn't all this handling done from the same walk to avoid carrying this state between them?
There was a problem hiding this comment.
In preorder, we walk each tree from any lcl var leaf up towards the root, but we don't always reach the root if we can see that the particular leaf escapes or doesn't escape at some intermediate node.
So at store, we don't know if we have built a model for what gets stored until we've finished visiting all the descendant nodes.
src/coreclr/jit/objectalloc.cpp
Outdated
| // Parameters have unknown initial values. | ||
| // | ||
| if (lclDsc->lvIsParam) |
There was a problem hiding this comment.
Do OSR locals need modelling here?
There was a problem hiding this comment.
We currently will only retype to TYP_BYREF in OSR methods, so we're excluding the OSR locals (and more) during the analysis.
We could model things more accurately now by giving each OSR local an unknown correction and removing the broader analysis exclusion. I'll push an update for this.
src/coreclr/jit/objectalloc.cpp
Outdated
| const unsigned rhsLclNum = IndexToLocal(rhsLclIndex); | ||
| isStackPointing = DoesLclVarPointToStack(rhsLclNum); |
There was a problem hiding this comment.
It seems like we do a lot of work to translate back and forth between "locals" and "indices", which feels a little bit confusing and unnecessary to me. Particularly the fact that "pseudo locals" also are considered "locals" and not just another type of abstract thing that has an index. Do they need to have "local" status?
There was a problem hiding this comment.
Can the entire loop here be done with BitVecOps::IsSubset(m_ConnGraphAdjacencyMatrix[lclIndex], m_definitelyStackPointingPointers)?
There was a problem hiding this comment.
Yeah, seems like we can avoid looping.
As for lclnum/index shuffling: I could possibly represent the abstract things with only indices and introduce a second set of index-based helpers to avoid some of the back and forth, but I'm not sure that would be an improvement.
| Compiler::fgWalkResult PostOrderVisit(GenTree** use, GenTree* user) | ||
| { | ||
| GenTree* const tree = *use; | ||
| if (tree->OperIsLocalStore()) |
There was a problem hiding this comment.
Why is this part done in post order?
There was a problem hiding this comment.
Same answer as above: #113808 (comment)
We need to know that all the descendant lcl var nodes have already tried to trace their paths up to this node.
There was a problem hiding this comment.
Ah ok, CanLclVarEscapeViaParentStack is called from this visitor in the preorder... Now things make more sense to me.
There was a problem hiding this comment.
I suppose I could move the current preorder processing to postorder too... will leave as-is for now.
There was a problem hiding this comment.
Yeah... Or even more radically this visitor could work similarly to local morph, tracking semantic information about operands on its own stack and escaping them from the parent instead of the child.
|
Latest diffs |
|
/ba-g Failures are azurelinux timeouts |
Instead, model all assignments to unescaped locals in the connection graph, and deduce the new type via graph analysis.
Update the INDEX_ADDR expansion to handle cases where the array is stack allocated (interior pointers are now TYP_I_IMPL, not TYP_BYREF).