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.

Sign up for GitHub

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

Updating genCodeForBinary to be VEX aware #1344

Merged
merged 2 commits into from
Jan 10, 2020
+186 −128
Merged

Updating genCodeForBinary to be VEX aware #1344

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
628ce55
Updating genCodeForBinary to be VEX aware
tannergooding Jan 7, 2020
dedbaf5
Resolving some PR feedback
tannergooding Jan 8, 2020
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
1 change: 1 addition & 0 deletions src/coreclr/src/jit/codegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1336,6 +1336,7 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX
#if defined(_TARGET_XARCH_)
void inst_RV_RV_IV(instruction ins, emitAttr size, regNumber reg1, regNumber reg2, unsigned ival);
void inst_RV_TT_IV(instruction ins, emitAttr attr, regNumber reg1, GenTree* rmOp, int ival);
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was conditioned under FEATURE_HW_INTRINSICS for the implementation, but not the declaration here. So, I did the minimal fixup to allow it to be available without `FEATURE_HW_INTRINSICS.

void inst_RV_RV_TT(instruction ins, emitAttr size, regNumber targetReg, regNumber op1Reg, GenTree* op2, bool isRMW);
#endif

void inst_RV_RR(instruction ins, emitAttr size, regNumber reg1, regNumber reg2);
Expand Down
12 changes: 12 additions & 0 deletions src/coreclr/src/jit/codegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -909,6 +909,18 @@ void CodeGen::genCodeForBinary(GenTreeOp* treeNode)
regNumber op1reg = op1->isUsedFromReg() ? op1->GetRegNum() : REG_NA;
regNumber op2reg = op2->isUsedFromReg() ? op2->GetRegNum() : REG_NA;

if (varTypeIsFloating(treeNode->TypeGet()))
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This just hijacks all the floating-point types and forwards to inst_RV_RV_TT, the emit helpers (such as emitIns_SIMD_R_R_R) deal with the difference between VEX and non-VEX for dst and op1Reg.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great - I think that's the right approach.

{
// floating-point addition, subtraction, multiplication, and division
// all have RMW semantics if VEX support is not available

bool isRMW = !compiler->canUseVexEncoding();
inst_RV_RV_TT(ins, emitTypeSize(treeNode), targetReg, op1reg, op2, isRMW);

genProduceReg(treeNode);
return;
}

GenTree* dst;
GenTree* src;

Expand Down
4 changes: 1 addition & 3 deletions src/coreclr/src/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -214,7 +214,6 @@ bool emitter::AreUpper32BitsZero(regNumber reg)
return false;
}

#ifdef FEATURE_HW_INTRINSICS
//------------------------------------------------------------------------
// IsDstSrcImmAvxInstruction: Checks if the instruction has a "reg, reg/mem, imm" or
// "reg/mem, reg, imm" form for the legacy, VEX, and EVEX
Expand Down Expand Up @@ -250,7 +249,6 @@ static bool IsDstSrcImmAvxInstruction(instruction ins)
return false;
}
}
#endif // FEATURE_HW_INTRINSICS

// -------------------------------------------------------------------
// Is4ByteSSEInstruction: Returns true if the SSE instruction is a 4-byte opcode.
Expand Down Expand Up @@ -5629,7 +5627,6 @@ void emitter::emitIns_AX_R(instruction ins, emitAttr attr, regNumber ireg, regNu
emitAdjustStackDepthPushPop(ins);
}

#ifdef FEATURE_HW_INTRINSICS
//------------------------------------------------------------------------
// emitIns_SIMD_R_R_I: emits the code for an instruction that takes a register operand, an immediate operand
// and that returns a value in register
Expand Down Expand Up @@ -5810,6 +5807,7 @@ void emitter::emitIns_SIMD_R_R_S(
}
}

#ifdef FEATURE_HW_INTRINSICS
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I only made the minimum number of emitIns_SIMD_* methods needed available outside FEATURE_HW_INTRINSICS as the others can't be encountered during normal codegen (and likely never will be).

//------------------------------------------------------------------------
// emitIns_SIMD_R_R_A_I: emits the code for a SIMD instruction that takes a register operand, a GenTreeIndir address,
// an immediate operand, and that returns a value in register
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/src/jit/emitxarch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -432,7 +432,6 @@ void emitIns_R_AX(instruction ins, emitAttr attr, regNumber ireg, regNumber reg,

void emitIns_AX_R(instruction ins, emitAttr attr, regNumber ireg, regNumber reg, unsigned mul, int disp);

#ifdef FEATURE_HW_INTRINSICS
void emitIns_SIMD_R_R_I(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, int ival);

void emitIns_SIMD_R_R_A(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, GenTreeIndir* indir);
Expand All @@ -443,6 +442,7 @@ void emitIns_SIMD_R_R_C(
void emitIns_SIMD_R_R_R(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, regNumber op2Reg);
void emitIns_SIMD_R_R_S(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, int varx, int offs);

#ifdef FEATURE_HW_INTRINSICS
void emitIns_SIMD_R_R_A_I(
instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, GenTreeIndir* indir, int ival);
void emitIns_SIMD_R_R_AR_I(
Expand Down
120 changes: 2 additions & 118 deletions src/coreclr/src/jit/hwintrinsiccodegenxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,133 +600,17 @@ void CodeGen::genHWIntrinsic_R_R_RM(GenTreeHWIntrinsic* node, instruction ins, e
void CodeGen::genHWIntrinsic_R_R_RM(
GenTreeHWIntrinsic* node, instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, GenTree* op2)
{
emitter* emit = GetEmitter();

// TODO-XArch-CQ: Commutative operations can have op1 be contained
// TODO-XArch-CQ: Non-VEX encoded instructions can have both ops contained

assert(targetReg != REG_NA);
assert(op1Reg != REG_NA);

if (op2->isContained() || op2->isUsedFromSpillTemp())
{
assert(HWIntrinsicInfo::SupportsContainment(node->gtHWIntrinsicId));
assertIsContainableHWIntrinsicOp(compiler->m_pLowering, node, op2);

TempDsc* tmpDsc = nullptr;
unsigned varNum = BAD_VAR_NUM;
unsigned offset = (unsigned)-1;

if (op2->isUsedFromSpillTemp())
{
assert(op2->IsRegOptional());

tmpDsc = getSpillTempDsc(op2);
varNum = tmpDsc->tdTempNum();
offset = 0;

regSet.tmpRlsTemp(tmpDsc);
}
else if (op2->isIndir() || op2->OperIsHWIntrinsic())
{
GenTree* addr;
GenTreeIndir* memIndir = nullptr;

if (op2->isIndir())
{
memIndir = op2->AsIndir();
addr = memIndir->Addr();
}
else
{
assert(op2->AsHWIntrinsic()->OperIsMemoryLoad());
assert(HWIntrinsicInfo::lookupNumArgs(op2->AsHWIntrinsic()) == 1);
addr = op2->gtGetOp1();
}

switch (addr->OperGet())
{
case GT_LCL_VAR_ADDR:
{
varNum = addr->AsLclVarCommon()->GetLclNum();
offset = 0;
break;
}

case GT_CLS_VAR_ADDR:
{
emit->emitIns_SIMD_R_R_C(ins, attr, targetReg, op1Reg, addr->AsClsVar()->gtClsVarHnd, 0);
return;
}

default:
{
GenTreeIndir load = indirForm(op2->TypeGet(), addr);

if (memIndir == nullptr)
{
// This is the HW intrinsic load case.
// Until we improve the handling of addressing modes in the emitter, we'll create a
// temporary GT_IND to generate code with.
memIndir = &load;
}
emit->emitIns_SIMD_R_R_A(ins, attr, targetReg, op1Reg, memIndir);
return;
}
}
}
else
{
switch (op2->OperGet())
{
case GT_LCL_FLD:
varNum = op2->AsLclFld()->GetLclNum();
offset = op2->AsLclFld()->GetLclOffs();
break;

case GT_LCL_VAR:
{
assert(op2->IsRegOptional() ||
!compiler->lvaTable[op2->AsLclVar()->GetLclNum()].lvIsRegCandidate());
varNum = op2->AsLclVar()->GetLclNum();
offset = 0;
break;
}

default:
unreached();
break;
}
}

// Ensure we got a good varNum and offset.
// We also need to check for `tmpDsc != nullptr` since spill temp numbers
// are negative and start with -1, which also happens to be BAD_VAR_NUM.
assert((varNum != BAD_VAR_NUM) || (tmpDsc != nullptr));
assert(offset != (unsigned)-1);

emit->emitIns_SIMD_R_R_S(ins, attr, targetReg, op1Reg, varNum, offset);
}
else
{
regNumber op2Reg = op2->GetRegNum();

if ((op1Reg != targetReg) && (op2Reg == targetReg) && node->isRMWHWIntrinsic(compiler))
{
// We have "reg2 = reg1 op reg2" where "reg1 != reg2" on a RMW intrinsic.
//
// For non-commutative intrinsics, we should have ensured that op2 was marked
// delay free in order to prevent it from getting assigned the same register
// as target. However, for commutative intrinsics, we can just swap the operands
// in order to have "reg2 = reg2 op reg1" which will end up producing the right code.

noway_assert(node->OperIsCommutative());
op2Reg = op1Reg;
op1Reg = targetReg;
}

emit->emitIns_SIMD_R_R_R(ins, attr, targetReg, op1Reg, op2Reg);
}
bool isRMW = node->isRMWHWIntrinsic(compiler);
inst_RV_RV_TT(ins, attr, targetReg, op1Reg, op2, isRMW);
}

//------------------------------------------------------------------------
Expand Down
Loading