From 6acd5e946f697ce9f3f15228e4359225aa570681 Mon Sep 17 00:00:00 2001 From: Carol Eidt Date: Thu, 12 Jul 2018 12:51:06 -0700 Subject: [PATCH] Ensure BuildUse uses the correct reg number `BuildUse` was setting the regNumber for all of the uses of a multi-reg node to the main/first regNum. This was missed because this results in a def/use conflict on that reg (the def was set correctly), which is generally resolved in favor of the def. The exception is when there is a kill of the register in between, in which case the use tries to allocate the register its been assigned, causing the `farthestRefPhysRegRecord != nullptr` assert (aka "a register can't be found to spill"). This fixes the specific issue, and adds additional asserts to identify future/additional such issues. The new asserts depend upon all the regNums being appropriately when/if any are set, which wasn't always the case prior to register allocation. Fix dotnet/coreclr#18153 Commit migrated from https://github.com/dotnet/coreclr/commit/b4470d75fa6ab4461607f2a0135cd118f466c2ef --- src/coreclr/src/jit/gentree.h | 42 +++++++++++++++++++++++++++++++++++++ src/coreclr/src/jit/lower.cpp | 6 +++++- src/coreclr/src/jit/lsra.cpp | 2 +- src/coreclr/src/jit/lsraarmarch.cpp | 2 +- src/coreclr/src/jit/lsrabuild.cpp | 23 +++++++++++++------- 5 files changed, 64 insertions(+), 11 deletions(-) diff --git a/src/coreclr/src/jit/gentree.h b/src/coreclr/src/jit/gentree.h index 3078b77..e7a522f 100644 --- a/src/coreclr/src/jit/gentree.h +++ b/src/coreclr/src/jit/gentree.h @@ -1721,6 +1721,9 @@ public: // Returns true if it is a node returning its value in more than one register inline bool IsMultiRegNode() const; + // Returns the regIndex'th register defined by a possibly-multireg node. + regNumber GetRegByIndex(int regIndex); + // Returns true if it is a GT_COPY or GT_RELOAD node inline bool IsCopyOrReload() const; @@ -6006,6 +6009,45 @@ inline bool GenTree::IsMultiRegNode() const return false; } +//----------------------------------------------------------------------------------- +// GetRegByIndex: Get a specific register, based on regIndex, that is produced +// by this node. +// +// Arguments: +// regIndex - which register to return (must be 0 for non-multireg nodes) +// +// Return Value: +// The register, if any, assigned to this index for this node. +// +inline regNumber GenTree::GetRegByIndex(int regIndex) +{ + if (regIndex == 0) + { + return gtRegNum; + } + if (IsMultiRegCall()) + { + return AsCall()->GetRegNumByIdx(regIndex); + } + +#if defined(_TARGET_ARM_) + if (OperIsMultiRegOp()) + { + return AsMultiRegOp()->GetRegNumByIdx(regIndex); + } + else if (OperIsPutArgSplit()) + { + return AsPutArgSplit()->GetRegNumByIdx(regIndex); + } + else if (OperIs(GT_COPY, GT_RELOAD)) + { + return AsCopyOrReload()->GetRegNumByIdx(regIndex); + } +#endif + assert(!"Invalid regIndex for GetRegFromMultiRegNode"); + return REG_NA; +} + //------------------------------------------------------------------------- // IsCopyOrReload: whether this is a GT_COPY or GT_RELOAD node. // diff --git a/src/coreclr/src/jit/lower.cpp b/src/coreclr/src/jit/lower.cpp index 8859c2f..f6a0b7c 100644 --- a/src/coreclr/src/jit/lower.cpp +++ b/src/coreclr/src/jit/lower.cpp @@ -1055,13 +1055,17 @@ GenTree* Lowering::NewPutArg(GenTreeCall* call, GenTree* arg, fgArgTabEntry* inf putArg = new (comp, GT_PUTARG_SPLIT) GenTreePutArgSplit(arg, info->slotNum PUT_STRUCT_ARG_STK_ONLY_ARG(info->numSlots), info->numRegs, call->IsFastTailCall(), call); - putArg->gtRegNum = info->regNum; // If struct argument is morphed to GT_FIELD_LIST node(s), // we can know GC info by type of each GT_FIELD_LIST node. // So we skip setting GC Pointer info. // GenTreePutArgSplit* argSplit = putArg->AsPutArgSplit(); + for (unsigned regIndex = 0; regIndex < info->numRegs; regIndex++) + { + argSplit->SetRegNumByIdx(info->getRegNum(regIndex), regIndex); + } + if (arg->OperGet() == GT_OBJ) { BYTE* gcLayout = nullptr; diff --git a/src/coreclr/src/jit/lsra.cpp b/src/coreclr/src/jit/lsra.cpp index c5afa1b..f6f749f 100644 --- a/src/coreclr/src/jit/lsra.cpp +++ b/src/coreclr/src/jit/lsra.cpp @@ -9109,7 +9109,7 @@ void LinearScan::TupleStyleDump(LsraTupleDumpMode mode) printf("\n Use:"); interval->microDump(); printf("(#%d)", currentRefPosition->rpNum); - if (currentRefPosition->isFixedRegRef) + if (currentRefPosition->isFixedRegRef && !interval->isInternal) { assert(genMaxOneBit(currentRefPosition->registerAssignment)); assert(lastFixedRegRefPos != nullptr); diff --git a/src/coreclr/src/jit/lsraarmarch.cpp b/src/coreclr/src/jit/lsraarmarch.cpp index dad8a75..87c0991 100644 --- a/src/coreclr/src/jit/lsraarmarch.cpp +++ b/src/coreclr/src/jit/lsraarmarch.cpp @@ -283,7 +283,7 @@ int LinearScan::BuildCall(GenTreeCall* call) assert(regCount == curArgTabEntry->numRegs); for (unsigned int i = 0; i < regCount; i++) { - BuildUse(argNode, genRegMask(argNode->gtRegNum), i); + BuildUse(argNode, genRegMask(argNode->AsPutArgSplit()->GetRegNumByIdx(i)), i); } srcCount += regCount; } diff --git a/src/coreclr/src/jit/lsrabuild.cpp b/src/coreclr/src/jit/lsrabuild.cpp index 3cd3008..3c2a266 100644 --- a/src/coreclr/src/jit/lsrabuild.cpp +++ b/src/coreclr/src/jit/lsrabuild.cpp @@ -401,7 +401,7 @@ void LinearScan::applyCalleeSaveHeuristics(RefPosition* rp) // incorrectly allocate that register. // TODO-CQ: This means that we may often require a copy at the use of this node's result. // This case could be moved to BuildRefPositionsForNode, at the point where the def RefPosition is -// created, causing a RefTypeFixedRef to be added at that location. This, however, results in +// created, causing a RefTypeFixedReg to be added at that location. This, however, results in // more PhysReg RefPositions (a throughput impact), and a large number of diffs that require // further analysis to determine benefit. // See Issue #11274. @@ -516,12 +516,16 @@ RefPosition* LinearScan::newRefPosition( { RefPosition* newRP = newRefPositionRaw(theLocation, theTreeNode, theRefType); - newRP->setReg(getRegisterRecord(reg)); + RegRecord* regRecord = getRegisterRecord(reg); + newRP->setReg(regRecord); newRP->registerAssignment = mask; newRP->setMultiRegIdx(0); newRP->setAllocateIfProfitable(false); + // We can't have two RefPositions on a RegRecord at the same location, unless they are different types. + assert((regRecord->lastRefPosition == nullptr) || (regRecord->lastRefPosition->nodeLocation < theLocation) || + (regRecord->lastRefPosition->refType != theRefType)); associateRefPosWithInterval(newRP); DBEXEC(VERBOSE, newRP->dump()); @@ -579,8 +583,9 @@ RefPosition* LinearScan::newRefPosition(Interval* theInterval, bool insertFixedRef = false; if (isFixedRegister) { - // Insert a RefTypeFixedReg for any normal def or use (not ParamDef or BB) - if (theRefType == RefTypeUse || theRefType == RefTypeDef) + // Insert a RefTypeFixedReg for any normal def or use (not ParamDef or BB), + // but not an internal use (it will already have a FixedRef for the def). + if ((theRefType == RefTypeDef) || ((theRefType == RefTypeUse) && !theInterval->isInternal)) { insertFixedRef = true; } @@ -2322,6 +2327,12 @@ RefPosition* LinearScan::BuildDef(GenTree* tree, regMaskTP dstCandidates, int mu { assert(!tree->isContained()); RegisterType type = getDefType(tree); + + if (dstCandidates != RBM_NONE) + { + assert((tree->gtRegNum == REG_NA) || (dstCandidates == genRegMask(tree->GetRegByIndex(multiRegIdx)))); + } + #ifdef FEATURE_MULTIREG_ARGS_OR_RET if (tree->TypeGet() == TYP_STRUCT) { @@ -2501,10 +2512,6 @@ RefPosition* LinearScan::BuildUse(GenTree* operand, regMaskTP candidates, int mu Interval* interval; bool regOptional = operand->IsRegOptional(); - if (operand->gtRegNum != REG_NA) - { - candidates = genRegMask(operand->gtRegNum); - } if (isCandidateLocalRef(operand)) { interval = getIntervalForLocalVarNode(operand->AsLclVarCommon()); -- 2.7.4