Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Fix min-opts spill of tree temp large vectors #22530

Merged
merged 2 commits into from
Feb 13, 2019
Merged

Fix min-opts spill of tree temp large vectors #22530

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
c5c1ce5
Fix min-opts spill of tree temp large vectors
CarolEidt Feb 11, 2019
987aa20
PR Feedback and some refactoring
CarolEidt Feb 12, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 1 addition & 3 deletions src/jit/lsra.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -989,9 +989,7 @@ class LinearScan : public LinearScanInterface
void buildRefPositionsForNode(GenTree* tree, BasicBlock* block, LsraLocation loc);

#if FEATURE_PARTIAL_SIMD_CALLEE_SAVE
VARSET_VALRET_TP buildUpperVectorSaveRefPositions(GenTree* tree,
LsraLocation currentLoc,
regMaskTP fpCalleeKillSet);
void buildUpperVectorSaveRefPositions(GenTree* tree, LsraLocation currentLoc, VARSET_VALARG_TP liveLargeVectors);
void buildUpperVectorRestoreRefPositions(GenTree* tree, LsraLocation currentLoc, VARSET_VALARG_TP liveLargeVectors);
#endif // FEATURE_PARTIAL_SIMD_CALLEE_SAVE

Expand Down
103 changes: 55 additions & 48 deletions src/jit/lsrabuild.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1306,34 +1306,26 @@ void LinearScan::buildInternalRegisterUses()
#if FEATURE_PARTIAL_SIMD_CALLEE_SAVE
//------------------------------------------------------------------------
// buildUpperVectorSaveRefPositions - Create special RefPositions for saving
// the upper half of a set of large vector.
// the upper half of a set of large vectors.
//
// Arguments:
// tree - The current node being handled
// currentLoc - The location of the current node
// fpCalleeKillSet - The set of registers killed by this node.
//
// Return Value: Returns the set of lclVars that are killed by this node, and therefore
// required RefTypeUpperVectorSaveDef RefPositions.
//
// Notes: The returned set contains any lclVars that were partially spilled, and is
// used by buildUpperVectorRestoreRefPositions.
// This is called by BuildDefsWithKills for any node that kills registers in the
// RBM_FLT_CALLEE_TRASH set. We actually need to find any calls that kill the upper-half
// of the callee-save vector registers.
// But we will use as a proxy any node that kills floating point registers.
// (Note that some calls are masquerading as other nodes at this point so we can't just check for calls.)
//
VARSET_VALRET_TP
LinearScan::buildUpperVectorSaveRefPositions(GenTree* tree, LsraLocation currentLoc, regMaskTP fpCalleeKillSet)
// tree - The current node being handled.
// currentLoc - The location of the current node.
// liveLargeVectors - The set of large vector lclVars live across 'tree'.
//
// Notes:
// At this time we create these RefPositions for any large vectors that are live across this node,
// though we don't yet know which, if any, will be in a callee-save register at this point.
// (If they're in a caller-save register, they will simply be fully spilled.)
// This method is called after all the use and kill RefPositions are created for 'tree' but before
// any definitions.
//
void LinearScan::buildUpperVectorSaveRefPositions(GenTree* tree,
LsraLocation currentLoc,
VARSET_VALARG_TP liveLargeVectors)
{
assert(enregisterLocalVars);
VARSET_TP liveLargeVectors(VarSetOps::MakeEmpty(compiler));
if (!VarSetOps::IsEmpty(compiler, largeVectorVars))
if (enregisterLocalVars && !VarSetOps::IsEmpty(compiler, largeVectorVars))
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this function does two things that needs to be split:

  1. a function that returns the set of lclVars (excluding temps) that are killed by this node;
  2. 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.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

{
assert((fpCalleeKillSet & RBM_FLT_CALLEE_TRASH) != RBM_NONE);
VarSetOps::AssignNoCopy(compiler, liveLargeVectors,
VarSetOps::Intersection(compiler, currentLiveVars, largeVectorVars));
VarSetOps::Iter iter(compiler, liveLargeVectors);
unsigned varIndex = 0;
while (iter.NextElem(&varIndex))
Expand All @@ -1350,31 +1342,34 @@ LinearScan::buildUpperVectorSaveRefPositions(GenTree* tree, LsraLocation current
tempInterval->relatedInterval = varInterval->relatedInterval;
varInterval->relatedInterval = tempInterval;
}
// For any non-lclVar intervals that are live at this point (i.e. in the DefList), we will also create
// a RefTypeUpperVectorSaveDef. For now these will all be spilled at this point, as we don't currently
// have a mechanism to communicate any non-lclVar intervals that need to be restored.
// TODO-CQ: Consider reworking this as part of addressing GitHub Issues #18144 and #17481.
for (RefInfoListNode *listNode = defList.Begin(), *end = defList.End(); listNode != end;
listNode = listNode->Next())
}
// For any non-lclVar large vector intervals that are live at this point (i.e. tree temps in the DefList),
// we will also create a RefTypeUpperVectorSaveDef. For now these will all be spilled at this point,
// as we don't currently have a mechanism to communicate any non-lclVar intervals that need to be restored.
// TODO-CQ: Consider reworking this as part of addressing GitHub Issues #18144 and #17481.
for (RefInfoListNode *listNode = defList.Begin(), *end = defList.End(); listNode != end;
listNode = listNode->Next())
{
var_types treeType = listNode->treeNode->TypeGet();
if (varTypeIsSIMD(treeType) && varTypeNeedsPartialCalleeSave(treeType))
{
var_types treeType = listNode->treeNode->TypeGet();
if (varTypeIsSIMD(treeType) && varTypeNeedsPartialCalleeSave(treeType))
{
RefPosition* pos = newRefPosition(listNode->ref->getInterval(), currentLoc, RefTypeUpperVectorSaveDef,
tree, RBM_FLT_CALLEE_SAVED);
}
RefPosition* pos = newRefPosition(listNode->ref->getInterval(), currentLoc, RefTypeUpperVectorSaveDef, tree,
RBM_FLT_CALLEE_SAVED);
}
}
return liveLargeVectors;
}

// buildUpperVectorRestoreRefPositions - Create special RefPositions for restoring
// the upper half of a set of large vectors.
//
// Arguments:
// tree - The current node being handled
// currentLoc - The location of the current node
// liveLargeVectors - The set of lclVars needing restores (returned by buildUpperVectorSaveRefPositions)
// tree - The current node being handled.
// currentLoc - The location of the current node.
// liveLargeVectors - The set of large vector lclVars live across 'tree'.
//
// Notes:
// This is called after all the RefPositions for 'tree' have been created, since the restore,
// if needed, will be inserted after the call.
//
void LinearScan::buildUpperVectorRestoreRefPositions(GenTree* tree,
LsraLocation currentLoc,
Expand Down Expand Up @@ -2553,20 +2548,32 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa
// Generate Kill RefPositions
assert(killMask == getKillSetForNode(tree));
#if FEATURE_PARTIAL_SIMD_CALLEE_SAVE
VARSET_TP liveLargeVectors(VarSetOps::UninitVal());
VARSET_TP liveLargeVectorVars(VarSetOps::UninitVal());
bool doLargeVectorRestore = false;
#endif // FEATURE_PARTIAL_SIMD_CALLEE_SAVE
if (killMask != RBM_NONE)
{
buildKillPositionsForNode(tree, currentLoc + 1, killMask);
#if FEATURE_PARTIAL_SIMD_CALLEE_SAVE
if (enregisterLocalVars && ((killMask & RBM_FLT_CALLEE_TRASH) != RBM_NONE))
// Build RefPositions to account for the fact that, even in a callee-save register, the upper half of any large
// vector will be killed by a call.
// We actually need to find any calls that kill the upper-half of the callee-save vector registers.
// But we will use as a proxy any node that kills floating point registers.
// (Note that some calls are masquerading as other nodes at this point so we can't just check for calls.)
// We call this unconditionally for such nodes, as we will create RefPositions for any large vector tree temps
// even if 'enregisterLocalVars' is false, or 'liveLargeVectors' is empty, though currently the allocation
// phase will fully (rather than partially) spill those, so we don't need to build the UpperVectorRestore
// RefPositions in that case.
//
if ((killMask & RBM_FLT_CALLEE_TRASH) != RBM_NONE)
{
// 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::AssignNoCopy(compiler, liveLargeVectorVars,
VarSetOps::Intersection(compiler, currentLiveVars, largeVectorVars));
}
buildUpperVectorSaveRefPositions(tree, currentLoc, liveLargeVectorVars);
doLargeVectorRestore = (enregisterLocalVars && !VarSetOps::IsEmpty(compiler, liveLargeVectorVars));
}
#endif // FEATURE_PARTIAL_SIMD_CALLEE_SAVE
}
Expand All @@ -2578,7 +2585,7 @@ void LinearScan::BuildDefsWithKills(GenTree* tree, int dstCount, regMaskTP dstCa
// Finally, generate the UpperVectorRestores
if (doLargeVectorRestore)
{
buildUpperVectorRestoreRefPositions(tree, currentLoc, liveLargeVectors);
buildUpperVectorRestoreRefPositions(tree, currentLoc, liveLargeVectorVars);
}
#endif // FEATURE_PARTIAL_SIMD_CALLEE_SAVE
}
Expand Down