Fix min-opts spill of tree temp large vectors #22530
Conversation
Even if we're not enregistering local vars, we may have large vectors live across a call that need to be spilled. Fix #22200
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs8 Build and Test |
|
@dotnet/jit-contrib PTAL |
|
#22311 Fixed this issue only for the case where we are also enregistering local vars. So, for minopts, or if there are no lclVars, this wasn't kicking in for the tree temps. |
|
@dotnet/jit-contrib ping Failing with: |
| VARSET_TP liveLargeVectors(VarSetOps::MakeEmpty(compiler)); | ||
| if (!VarSetOps::IsEmpty(compiler, largeVectorVars)) | ||
| if (enregisterLocalVars && !VarSetOps::IsEmpty(compiler, largeVectorVars)) |
There was a problem hiding this comment.
Could you please clarify some names, overall we have variables and temps, largeVectorVars refers to variables, enregisterLocalVars also refers to variables, is it correct?
Could we have non-empty largeVectorVars (that doesn't include temps, only vars) when enregisterLocalVars is false? Sound like we can't, then can you move the deleted assert under this condition?
There was a problem hiding this comment.
Looks like this function does two things that needs to be split:
- a function that returns the set of lclVars (excluding temps) that are killed by this node;
- a function that create special RefPositions for saving the upper half of a set of large vector (including temps).
and the same for buildUpperVectorRestoreRefPositions.
If you do not want to split them now, could you please update their headers to include that they not only create re positions but also save/restore vars.
There was a problem hiding this comment.
It would make sense to split out the determination of the live large vector lclVars (including lclVar temps, which are the same as lclVars from the perspective of LSRA) from the building of the RefPositions). I'd prefer to do that as a separate refactor.
Note, though, that this method doesn't actually save or restore anything, it just creates the RefPositions that may cause their upper half to be saved & restored, at allocation time, if they reside in a callee-save register.
There was a problem hiding this comment.
the determination of the live large vector lclVars (including lclVar temps, which are the same as lclVars from the perspective of LSRA)
Do you mean liveLargeVectors that the method returns? We calculate liveLargeVectors only under if (enregisterLocalVars && !VarSetOps::IsEmpty(compiler, largeVectorVars)) as I see. How can it include lclVar temp when we iterate only through vars? Could you please rename liveLargeVectors to liveLargeVectorsVars here as well?
Note that the return value is calculated in the first part of this function and the second part does not affect it at all.
src/jit/lsrabuild.cpp
Outdated
| @@ -1376,6 +1375,10 @@ LinearScan::buildUpperVectorSaveRefPositions(GenTree* tree, LsraLocation current | |||
| // currentLoc - The location of the current node | |||
| // liveLargeVectors - The set of lclVars needing restores (returned by buildUpperVectorSaveRefPositions) | |||
There was a problem hiding this comment.
another question about names, could you please rename it to liveLargeVectorsVars?
src/jit/lsrabuild.cpp
Outdated
| { | ||
| // Build RefPositions for saving any live large vectors. | ||
| // This must be done after the kills, so that we know which large vectors are still live. | ||
| VarSetOps::AssignNoCopy(compiler, liveLargeVectors, | ||
| buildUpperVectorSaveRefPositions(tree, currentLoc + 1, killMask)); | ||
| doLargeVectorRestore = true; | ||
| if (enregisterLocalVars && !VarSetOps::IsEmpty(compiler, liveLargeVectors)) |
There was a problem hiding this comment.
The same comment about adding assert(enregisterLocalVars) under if.
There was a problem hiding this comment.
Currently the lclVar sets are not initialized if enregisterLocalVars is false, so we can't move the condition to an assert (i.e. we need to check enregisterLocalVars before we test whether the set is empty).
No we don't have a test for this change. It is generally not easy to find tests for LSRA issues, but the |
|
@dotnet-bot test Ubuntu16.04 arm64 Cross Checked jitstress2_jitstressregs8 Build and Test |
* Fix min-opts spill of tree temp large vectors Even if we're not enregistering local vars, we may have large vectors live across a call that need to be spilled. Fix dotnet/coreclr#22200 Commit migrated from dotnet/coreclr@fc70d03
Even if we're not enregistering local vars, we may have large vectors live across a call that need to be spilled.
Fix #22200