Skip to content

Commit

Permalink
Fix an over-constrained use of a byte reg (#41004)
Browse files Browse the repository at this point in the history
* Fix an over-constrained use of a byte reg

Fix #40963

* PR Feedback
  • Loading branch information
CarolEidt authored Aug 21, 2020
1 parent cd2eed7 commit a5b491c
Show file tree
Hide file tree
Showing 2 changed files with 50 additions and 2 deletions.
51 changes: 49 additions & 2 deletions src/coreclr/src/jit/lsraxarch.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1280,6 +1280,11 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode)
regMaskTP srcRegMask = RBM_NONE;
regMaskTP sizeRegMask = RBM_NONE;

RefPosition* internalIntDef = nullptr;
#ifdef TARGET_X86
bool internalIsByte = false;
#endif

if (blkNode->OperIsInitBlkOp())
{
if (src->OperIs(GT_INIT_VAL))
Expand Down Expand Up @@ -1359,10 +1364,11 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode)
if ((size & 1) != 0)
{
// We'll need to store a byte so a byte register is needed on x86.
regMask = allByteRegs();
regMask = allByteRegs();
internalIsByte = true;
}
#endif
buildInternalIntRegisterDefForNode(blkNode, regMask);
internalIntDef = buildInternalIntRegisterDefForNode(blkNode, regMask);
}

if (size >= XMM_REGSIZE_BYTES)
Expand Down Expand Up @@ -1436,9 +1442,30 @@ int LinearScan::BuildBlockStore(GenTreeBlk* blkNode)
BuildUse(blkNode->AsDynBlk()->gtDynamicSize, sizeRegMask);
}

#ifdef TARGET_X86
// If we require a byte register on x86, we may run into an over-constrained situation
// if we have BYTE_REG_COUNT or more uses (currently, it can be at most 4, if both the
// source and destination have base+index addressing).
// This is because the byteable register requirement doesn't "reserve" a specific register,
// and it would be possible for the incoming sources to all be occupying the byteable
// registers, leaving none free for the internal register.
// In this scenario, we will require rax to ensure that it is reserved and available.
// We need to make that modification prior to building the uses for the internal register,
// so that when we create the use we will also create the RefTypeFixedRef on the RegRecord.
// We don't expect a useCount of more than 3 for the initBlk case, so we haven't set
// internalIsByte in that case above.
assert((useCount < BYTE_REG_COUNT) || !blkNode->OperIsInitBlkOp());
if (internalIsByte && (useCount >= BYTE_REG_COUNT))
{
noway_assert(internalIntDef != nullptr);
internalIntDef->registerAssignment = RBM_RAX;
}
#endif

buildInternalRegisterUses();
regMaskTP killMask = getKillSetForBlockStore(blkNode);
BuildDefsWithKills(blkNode, 0, RBM_NONE, killMask);

return useCount;
}

Expand Down Expand Up @@ -1594,6 +1621,15 @@ int LinearScan::BuildPutArgStk(GenTreePutArgStk* putArgStk)

srcCount = BuildOperandUses(src);
buildInternalRegisterUses();

#ifdef TARGET_X86
// There are only 4 (BYTE_REG_COUNT) byteable registers on x86. If we require a byteable internal register,
// we must have less than BYTE_REG_COUNT sources.
// If we have BYTE_REG_COUNT or more sources, and require a byteable internal register, we need to reserve
// one explicitly (see BuildBlockStore()).
assert(srcCount < BYTE_REG_COUNT);
#endif

return srcCount;
}
#endif // FEATURE_PUT_STRUCT_ARG_STK
Expand Down Expand Up @@ -2728,6 +2764,7 @@ int LinearScan::BuildIndir(GenTreeIndir* indirTree)
}
}
}

#ifdef FEATURE_SIMD
if (varTypeIsSIMD(indirTree))
{
Expand All @@ -2736,6 +2773,16 @@ int LinearScan::BuildIndir(GenTreeIndir* indirTree)
buildInternalRegisterUses();
#endif // FEATURE_SIMD

#ifdef TARGET_X86
// There are only BYTE_REG_COUNT byteable registers on x86. If we have a source that requires
// such a register, we must have no more than BYTE_REG_COUNT sources.
// If we have more than BYTE_REG_COUNT sources, and require a byteable register, we need to reserve
// one explicitly (see BuildBlockStore()).
// (Note that the assert below doesn't count internal registers because we only have
// floating point internal registers, if any).
assert(srcCount <= BYTE_REG_COUNT);
#endif

if (indirTree->gtOper != GT_STOREIND)
{
BuildDef(indirTree);
Expand Down
1 change: 1 addition & 0 deletions src/coreclr/src/jit/target.h
Original file line number Diff line number Diff line change
Expand Up @@ -1598,6 +1598,7 @@ C_ASSERT((FEATURE_TAILCALL_OPT == 0) || (FEATURE_FASTTAILCALL == 1));

#if CPU_HAS_BYTE_REGS
#define RBM_BYTE_REGS (RBM_EAX|RBM_ECX|RBM_EDX|RBM_EBX)
#define BYTE_REG_COUNT 4
#define RBM_NON_BYTE_REGS (RBM_ESI|RBM_EDI)
#else
#define RBM_BYTE_REGS RBM_ALLINT
Expand Down

0 comments on commit a5b491c

Please sign in to comment.