Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix spilling of MUL_LONG on x86 and multi-reg HWIs #73079

Merged
merged 3 commits into from
Sep 27, 2022
Merged

Fix spilling of MUL_LONG on x86 and multi-reg HWIs #73079

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2b04d12
Spilling fixes
SingleAccretion Jul 29, 2022
1ce6169
Unspilling fixes
SingleAccretion Jul 29, 2022
43c4cea
Fix multi-reg HWIs
SingleAccretion Jul 29, 2022
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
55 changes: 7 additions & 48 deletions src/coreclr/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2148,64 +2148,23 @@ void CodeGen::genProduceReg(GenTree* tree)
}
else
{
// In case of multi-reg call node, spill flag on call node
// indicates that one or more of its allocated regs need to
// be spilled. Call node needs to be further queried to
// know which of its result regs needs to be spilled.
if (tree->IsMultiRegCall())
if (tree->IsMultiRegNode())
{
GenTreeCall* call = tree->AsCall();
const ReturnTypeDesc* retTypeDesc = call->GetReturnTypeDesc();
const unsigned regCount = retTypeDesc->GetReturnRegCount();
// In case of multi-reg node, spill flag on it indicates that one or more of its allocated regs need to
// be spilled, and it needs to be further queried to know which of its result regs needs to be spilled.
const unsigned regCount = tree->GetMultiRegCount(compiler);

for (unsigned i = 0; i < regCount; ++i)
{
GenTreeFlags flags = call->GetRegSpillFlagByIdx(i);
GenTreeFlags flags = tree->GetRegSpillFlagByIdx(i);
if ((flags & GTF_SPILL) != 0)
{
regNumber reg = call->GetRegNumByIdx(i);
regSet.rsSpillTree(reg, call, i);
regNumber reg = tree->GetRegByIndex(i);
regSet.rsSpillTree(reg, tree, i);
gcInfo.gcMarkRegSetNpt(genRegMask(reg));
}
}
}
#if FEATURE_ARG_SPLIT
else if (tree->OperIsPutArgSplit())
{
assert(compFeatureArgSplit());
GenTreePutArgSplit* argSplit = tree->AsPutArgSplit();
unsigned regCount = argSplit->gtNumRegs;

for (unsigned i = 0; i < regCount; ++i)
{
GenTreeFlags flags = argSplit->GetRegSpillFlagByIdx(i);
if ((flags & GTF_SPILL) != 0)
{
regNumber reg = argSplit->GetRegNumByIdx(i);
regSet.rsSpillTree(reg, argSplit, i);
gcInfo.gcMarkRegSetNpt(genRegMask(reg));
}
}
}
#ifdef TARGET_ARM
else if (compFeatureArgSplit() && tree->OperIsMultiRegOp())
{
GenTreeMultiRegOp* multiReg = tree->AsMultiRegOp();
unsigned regCount = multiReg->GetRegCount();

for (unsigned i = 0; i < regCount; ++i)
{
GenTreeFlags flags = multiReg->GetRegSpillFlagByIdx(i);
if ((flags & GTF_SPILL) != 0)
{
regNumber reg = multiReg->GetRegNumByIdx(i);
regSet.rsSpillTree(reg, multiReg, i);
gcInfo.gcMarkRegSetNpt(genRegMask(reg));
}
}
}
#endif // TARGET_ARM
#endif // FEATURE_ARG_SPLIT
else
{
regSet.rsSpillTree(tree->GetRegNum(), tree);
Expand Down
2 changes: 0 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18616,7 +18616,6 @@ bool GenTreeSIMD::OperIsMemoryLoad() const
return GetSIMDIntrinsicId() == SIMDIntrinsicInitArray;
}

// TODO-Review: why are layouts not compared here?
/* static */ bool GenTreeSIMD::Equals(GenTreeSIMD* op1, GenTreeSIMD* op2)
{
return (op1->TypeGet() == op2->TypeGet()) && (op1->GetSIMDIntrinsicId() == op2->GetSIMDIntrinsicId()) &&
Expand Down Expand Up @@ -22837,7 +22836,6 @@ void GenTreeHWIntrinsic::SetHWIntrinsicId(NamedIntrinsic intrinsicId)
gtHWIntrinsicId = intrinsicId;
}

// TODO-Review: why are layouts not compared here?
/* static */ bool GenTreeHWIntrinsic::Equals(GenTreeHWIntrinsic* op1, GenTreeHWIntrinsic* op2)
{
return (op1->TypeGet() == op2->TypeGet()) && (op1->GetHWIntrinsicId() == op2->GetHWIntrinsicId()) &&
Expand Down
106 changes: 86 additions & 20 deletions src/coreclr/jit/gentree.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1860,6 +1860,9 @@ struct GenTree
// Returns the GTF flag equivalent for the regIndex'th register of a multi-reg node.
GenTreeFlags GetRegSpillFlagByIdx(int regIndex) const;

// Sets the GTF flag equivalent for the regIndex'th register of a multi-reg node.
void SetRegSpillFlagByIdx(GenTreeFlags flags, int regIndex);

// Last-use information for either GenTreeLclVar or GenTreeCopyOrReload nodes.
private:
GenTreeFlags GetLastUseBit(int regIndex) const;
Expand Down Expand Up @@ -5973,12 +5976,12 @@ class IntrinsicNodeBuilder final
struct GenTreeJitIntrinsic : public GenTreeMultiOp
{
protected:
GenTree* gtInlineOperands[2];
uint16_t gtLayoutNum;
unsigned char gtAuxiliaryJitType; // For intrinsics than need another type (e.g. Avx2.Gather* or SIMD (by element))
regNumberSmall gtOtherReg; // For intrinsics that return 2 registers
unsigned char gtSimdBaseJitType; // SIMD vector base JIT type
unsigned char gtSimdSize; // SIMD vector size in bytes, use 0 for scalar intrinsics
GenTree* gtInlineOperands[2];
regNumberSmall gtOtherReg; // The second register for multi-reg intrinsics.
MultiRegSpillFlags gtSpillFlags; // Spill flags for multi-reg intrinsics.
unsigned char gtAuxiliaryJitType; // For intrinsics than need another type (e.g. Avx2.Gather* or SIMD (by element))
unsigned char gtSimdBaseJitType; // SIMD vector base JIT type
unsigned char gtSimdSize; // SIMD vector size in bytes, use 0 for scalar intrinsics

#if defined(FEATURE_SIMD)
union {
Expand All @@ -5990,26 +5993,25 @@ struct GenTreeJitIntrinsic : public GenTreeMultiOp
#endif

public:
unsigned GetLayoutNum() const
regNumber GetOtherReg() const
{
return gtLayoutNum;
return (regNumber)gtOtherReg;
}

void SetLayoutNum(unsigned layoutNum)
void SetOtherReg(regNumber reg)
{
assert(FitsIn<uint16_t>(layoutNum));
gtLayoutNum = static_cast<uint16_t>(layoutNum);
gtOtherReg = (regNumberSmall)reg;
assert(gtOtherReg == reg);
}

regNumber GetOtherReg() const
GenTreeFlags GetRegSpillFlagByIdx(unsigned idx) const
{
return (regNumber)gtOtherReg;
return GetMultiRegSpillFlagsByIdx(gtSpillFlags, idx);
}

void SetOtherReg(regNumber reg)
void SetRegSpillFlagByIdx(GenTreeFlags flags, unsigned idx)
{
gtOtherReg = (regNumberSmall)reg;
assert(gtOtherReg == reg);
gtSpillFlags = SetMultiRegSpillFlagsByIdx(gtSpillFlags, flags, idx);
}

CorInfoType GetAuxiliaryJitType() const
Expand Down Expand Up @@ -6085,9 +6087,9 @@ struct GenTreeJitIntrinsic : public GenTreeMultiOp
unsigned simdSize,
Operands... operands)
: GenTreeMultiOp(oper, type, allocator, gtInlineOperands DEBUGARG(false), operands...)
, gtLayoutNum(0)
, gtAuxiliaryJitType(CORINFO_TYPE_UNDEF)
, gtOtherReg(REG_NA)
, gtSpillFlags(0)
, gtAuxiliaryJitType(CORINFO_TYPE_UNDEF)
, gtSimdBaseJitType((unsigned char)simdBaseJitType)
, gtSimdSize((unsigned char)simdSize)
, gtHWIntrinsicId(NI_Illegal)
Expand All @@ -6113,9 +6115,9 @@ struct GenTreeJitIntrinsic : public GenTreeMultiOp
nodeBuilder.GetBuiltOperands(),
nodeBuilder.GetOperandCount(),
gtInlineOperands DEBUGARG(false))
, gtLayoutNum(0)
, gtAuxiliaryJitType(CORINFO_TYPE_UNDEF)
, gtOtherReg(REG_NA)
, gtSpillFlags(0)
, gtAuxiliaryJitType(CORINFO_TYPE_UNDEF)
, gtSimdBaseJitType((unsigned char)simdBaseJitType)
, gtSimdSize((unsigned char)simdSize)
, gtHWIntrinsicId(NI_Illegal)
Expand Down Expand Up @@ -9067,6 +9069,13 @@ inline GenTreeFlags GenTree::GetRegSpillFlagByIdx(int regIndex) const
#endif // !defined(TARGET_64BIT)
#endif // FEATURE_MULTIREG_RET

#ifdef FEATURE_HW_INTRINSICS
if (OperIsHWIntrinsic())
{
return AsHWIntrinsic()->GetRegSpillFlagByIdx(regIndex);
}
#endif // FEATURE_HW_INTRINSICS

if (OperIsScalarLocal())
{
return AsLclVar()->GetRegSpillFlagByIdx(regIndex);
Expand All @@ -9076,6 +9085,63 @@ inline GenTreeFlags GenTree::GetRegSpillFlagByIdx(int regIndex) const
return GTF_EMPTY;
}

//-----------------------------------------------------------------------------------
// SetRegSpillFlagByIdx: Set a specific register's spill flags, based on regIndex,
// for this multi-reg node.
//
// Arguments:
// flags - the flags to set
// regIndex - which register's spill flags to set
//
// Notes:
// This must be a multireg node and 'regIndex' must be a valid index for this node.
// This method takes the GTF "equivalent" flags and sets the packed flags on the
// multireg node.
//
inline void GenTree::SetRegSpillFlagByIdx(GenTreeFlags flags, int regIndex)
{
#if FEATURE_MULTIREG_RET
if (IsMultiRegCall())
{
AsCall()->SetRegSpillFlagByIdx(flags, regIndex);
return;
}

#if FEATURE_ARG_SPLIT
if (OperIsPutArgSplit())
{
AsPutArgSplit()->SetRegSpillFlagByIdx(flags, regIndex);
return;
}
#endif // FEATURE_ARG_SPLIT

#if !defined(TARGET_64BIT)
if (OperIsMultiRegOp())
{
AsMultiRegOp()->SetRegSpillFlagByIdx(flags, regIndex);
return;
}
#endif // !defined(TARGET_64BIT)

#endif // FEATURE_MULTIREG_RET

#ifdef FEATURE_HW_INTRINSICS
if (OperIsHWIntrinsic())
{
AsHWIntrinsic()->SetRegSpillFlagByIdx(flags, regIndex);
return;
}
#endif // FEATURE_HW_INTRINSICS

if (OperIsScalarLocal())
{
AsLclVar()->SetRegSpillFlagByIdx(flags, regIndex);
return;
}

assert(!"Invalid node type for SetRegSpillFlagByIdx");
}

//-----------------------------------------------------------------------------------
// GetLastUseBit: Get the last use bit for regIndex
//
Expand Down
23 changes: 6 additions & 17 deletions src/coreclr/jit/lsra.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4665,7 +4665,11 @@ void LinearScan::allocateRegisters()
{
assert(lastAllocatedRefPosition->registerAssignment != RBM_NONE);
RegRecord* regRecord = lastAllocatedRefPosition->getInterval()->assignedReg;

INDEBUG(activeRefPosition = lastAllocatedRefPosition);
unassignPhysReg(regRecord, lastAllocatedRefPosition);
INDEBUG(activeRefPosition = nullptr);

// Now set lastAllocatedRefPosition to null, so that we don't try to spill it again
lastAllocatedRefPosition = nullptr;
}
Expand Down Expand Up @@ -6950,25 +6954,10 @@ void LinearScan::resolveRegisters()
// register specified by multi-reg index of current RefPosition.
// Note that the spill flag on treeNode indicates that one or
// more its allocated registers are in that state.
if (treeNode->IsMultiRegCall())
{
GenTreeCall* call = treeNode->AsCall();
call->SetRegSpillFlagByIdx(GTF_SPILL, currentRefPosition->getMultiRegIdx());
}
#if FEATURE_ARG_SPLIT
else if (treeNode->OperIsPutArgSplit())
if (treeNode->IsMultiRegNode())
{
GenTreePutArgSplit* splitArg = treeNode->AsPutArgSplit();
splitArg->SetRegSpillFlagByIdx(GTF_SPILL, currentRefPosition->getMultiRegIdx());
treeNode->SetRegSpillFlagByIdx(GTF_SPILL, currentRefPosition->getMultiRegIdx());
}
#ifdef TARGET_ARM
else if (compFeatureArgSplit() && treeNode->OperIsMultiRegOp())
{
GenTreeMultiRegOp* multiReg = treeNode->AsMultiRegOp();
multiReg->SetRegSpillFlagByIdx(GTF_SPILL, currentRefPosition->getMultiRegIdx());
}
#endif // TARGET_ARM
#endif // FEATURE_ARG_SPLIT
}

// If the value is reloaded or moved to a different register, we need to insert
Expand Down
Loading