Skip to content

Commit

Permalink
Fix spilling of MUL_LONG on x86 and multi-reg HWIs (dotnet#73079)
Browse files Browse the repository at this point in the history 
* Spilling fixes

* Unspilling fixes

* Fix multi-reg HWIs
  • Loading branch information
@SingleAccretion
SingleAccretion authored Sep 27, 2022
1 parent 7fc8fb5 commit ee10ed1
Show file tree
Hide file tree
Showing 5 changed files with 133 additions and 200 deletions.
55 changes: 7 additions & 48 deletions src/coreclr/jit/codegenlinear.cpp
View file
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
Original file line number Diff line number Diff line change
Expand Up @@ -18567,7 +18567,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 @@ -22816,7 +22815,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
Original file line number Diff line number Diff line change
Expand Up @@ -1861,6 +1861,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 @@ -5979,12 +5982,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 @@ -5996,26 +5999,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 @@ -6091,9 +6093,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 @@ -6119,9 +6121,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 @@ -9073,6 +9075,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 @@ -9082,6 +9091,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
Original file line number Diff line number Diff line change
Expand Up @@ -4666,7 +4666,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 @@ -6951,25 +6955,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

0 comments on commit ee10ed1

Please sign in to comment.