Skip to content

Commit

Permalink
Fix SIMD data overallocation (dotnet#71043)
Browse files Browse the repository at this point in the history
The JIT was allocating 16 bytes for TYP_SIMD8 static data loads,
and marking the data section as requiring 16 byte alignment as
well. Fix this to not allocate more than necessary, or over align.
  • Loading branch information
BruceForstall authored Jun 21, 2022
1 parent bb17fb5 commit 99e58d5
Show file tree
Hide file tree
Showing 3 changed files with 24 additions and 7 deletions.
18 changes: 15 additions & 3 deletions src/coreclr/jit/emit.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7275,7 +7275,12 @@ UNATIVE_OFFSET emitter::emitDataGenBeg(unsigned size, unsigned alignment, var_ty
}

assert((secOffs % alignment) == 0);
emitConsDsc.alignment = max(emitConsDsc.alignment, alignment);
if (emitConsDsc.alignment < alignment)
{
JITDUMP("Increasing data section alignment from %u to %u for type %s\n", emitConsDsc.alignment, alignment,
varTypeName(dataType));
emitConsDsc.alignment = alignment;
}

/* Advance the current offset */
emitConsDsc.dsdOffs += size;
Expand Down Expand Up @@ -7673,7 +7678,7 @@ void emitter::emitOutputDataSec(dataSecDsc* sec, BYTE* dst)

if (emitComp->opts.disAsm)
{
emitDispDataSec(sec);
emitDispDataSec(sec, dst);
}

unsigned secNum = 0;
Expand Down Expand Up @@ -7794,25 +7799,32 @@ void emitter::emitOutputDataSec(dataSecDsc* sec, BYTE* dst)
//
// Arguments:
// section - the data section description
// dst - address of the data section
//
// Notes:
// The output format attempts to mirror typical assembler syntax.
// Data section entries lack type information so float/double entries
// are displayed as if they are integers/longs.
//
void emitter::emitDispDataSec(dataSecDsc* section)
void emitter::emitDispDataSec(dataSecDsc* section, BYTE* dst)
{
printf("\n");

unsigned offset = 0;

for (dataSection* data = section->dsdList; data != nullptr; data = data->dsNext)
{
if (emitComp->opts.disAddr)
{
printf("; @" FMT_ADDR "\n", DBG_ADDR(dst));
}

const char* labelFormat = "%-7s";
char label[64];
sprintf_s(label, ArrLen(label), "RWD%02u", offset);
printf(labelFormat, label);
offset += data->dsSize;
dst += data->dsSize;

if ((data->dsType == dataSection::blockRelative32) || (data->dsType == dataSection::blockAbsoluteAddr))
{
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/emit.h
Original file line number Diff line number Diff line change
Expand Up @@ -2555,7 +2555,7 @@ class emitter

void emitOutputDataSec(dataSecDsc* sec, BYTE* dst);
#ifdef DEBUG
void emitDispDataSec(dataSecDsc* section);
void emitDispDataSec(dataSecDsc* section, BYTE* dst);
#endif

/************************************************************************/
Expand Down
11 changes: 8 additions & 3 deletions src/coreclr/jit/lower.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -7384,6 +7384,7 @@ void Lowering::LowerSIMD(GenTreeSIMD* simdNode)

if (arg->IsCnsFltOrDbl())
{
noway_assert(constArgCount < ArrLen(constArgValues));
constArgValues[constArgCount] = static_cast<float>(arg->AsDblCon()->gtDconVal);
constArgCount++;
}
Expand All @@ -7396,10 +7397,14 @@ void Lowering::LowerSIMD(GenTreeSIMD* simdNode)
BlockRange().Remove(arg);
}

assert(sizeof(constArgValues) == 16);
// For SIMD12, even though there might be 12 bytes of constants, we need to store 16 bytes of data
// since we've bashed the node the TYP_SIMD16 and do a 16-byte indirection.
assert(varTypeIsSIMD(simdNode));
const unsigned cnsSize = genTypeSize(simdNode);
assert(cnsSize <= sizeof(constArgValues));

unsigned cnsSize = sizeof(constArgValues);
unsigned cnsAlign = (comp->compCodeOpt() != Compiler::SMALL_CODE) ? cnsSize : 1;
const unsigned cnsAlign =
(comp->compCodeOpt() != Compiler::SMALL_CODE) ? cnsSize : emitter::dataSection::MIN_DATA_ALIGN;

CORINFO_FIELD_HANDLE hnd =
comp->GetEmitter()->emitBlkConst(constArgValues, cnsSize, cnsAlign, simdNode->GetSimdBaseType());
Expand Down

0 comments on commit 99e58d5

Please sign in to comment.