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.

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

[release/5.0] Fix an over-constrained use of a byte reg #41642

Merged
merged 2 commits into from
Sep 1, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
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