Skip to content
This repository has been archived by the owner on Jan 23, 2023. It is now read-only.

Handle addressing modes for HW intrinsics #22944

Merged
merged 5 commits into from
Mar 26, 2019
Merged

Handle addressing modes for HW intrinsics #22944

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0ae8f19
Handle addressing modes for HW intrinsics
CarolEidt Mar 1, 2019
f1ae7f8
Fix Broadcast
CarolEidt Mar 21, 2019
a8e5f21
Enhance the test case.
CarolEidt Mar 21, 2019
a5d0992
Add checks for ISA support
CarolEidt Mar 21, 2019
2384672
Fix guard for NonTemporal load
CarolEidt Mar 25, 2019
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
5 changes: 5 additions & 0 deletions src/jit/codegen.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -95,6 +95,7 @@ class CodeGen : public CodeGenInterface
static bool genShouldRoundFP();

GenTreeIndir indirForm(var_types type, GenTree* base);
GenTreeStoreInd storeIndirForm(var_types type, GenTree* base, GenTree* data);

GenTreeIntCon intForm(var_types type, ssize_t value);

Expand Down Expand Up @@ -1042,6 +1043,9 @@ XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX

void genConsumeRegs(GenTree* tree);
void genConsumeOperands(GenTreeOp* tree);
#ifdef FEATURE_HW_INTRINSICS
void genConsumeHWIntrinsicOperands(GenTreeHWIntrinsic* tree);
#endif // FEATURE_HW_INTRINSICS
void genEmitGSCookieCheck(bool pushReg);
void genSetRegToIcon(regNumber reg, ssize_t val, var_types type = TYP_INT, insFlags flags = INS_FLAGS_DONT_CARE);
void genCodeForShift(GenTree* tree);
Expand Down Expand Up @@ -1311,6 +1315,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);
#endif

void inst_RV_RR(instruction ins, emitAttr size, regNumber reg1, regNumber reg2);
Expand Down
11 changes: 11 additions & 0 deletions src/jit/codegencommon.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11296,6 +11296,17 @@ GenTreeIndir CodeGen::indirForm(var_types type, GenTree* base)
return i;
}

//------------------------------------------------------------------------
// indirForm: Make a temporary indir we can feed to pattern matching routines
// in cases where we don't want to instantiate all the indirs that happen.
//
GenTreeStoreInd CodeGen::storeIndirForm(var_types type, GenTree* base, GenTree* data)
{
GenTreeStoreInd i(type, base, data);
i.gtRegNum = REG_NA;
return i;
}

//------------------------------------------------------------------------
// intForm: Make a temporary int we can feed to pattern matching routines
// in cases where we don't want to instantiate.
Expand Down
77 changes: 68 additions & 9 deletions src/jit/codegenlinear.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1333,12 +1333,27 @@ void CodeGen::genConsumeRegs(GenTree* tree)
// Update the life of the lcl var.
genUpdateLife(tree);
}
#endif // _TARGET_XARCH_
else if (tree->OperIsInitVal())
#ifdef FEATURE_HW_INTRINSICS
else if (tree->OperIs(GT_HWIntrinsic))
Copy link

Choose a reason for hiding this comment

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

GT_HWIntrinsic is already handled a few lines below.

Copy link
Author

Choose a reason for hiding this comment

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

Thanks, I missed that. I don't believe that we have (or will have) any Arm64 intrinsics that can be contained, so I'll remove the one further down.

Copy link
Member

Choose a reason for hiding this comment

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

Did this get addressed? I don't see the matching removal

Copy link
Author

Choose a reason for hiding this comment

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

It's a bit hard to see, with the way the diffs are shown, but if you look below, this line was deleted:

        else if (tree->OperIsHWIntrinsic())

and the genConsumeReg below it is actually the one for the OperIsInitVal case.

{
genConsumeReg(tree->gtGetOp1());
// Only load/store HW intrinsics can be contained (and the address may also be contained).
HWIntrinsicCategory category = HWIntrinsicInfo::lookupCategory(tree->AsHWIntrinsic()->gtHWIntrinsicId);
assert((category == HW_Category_MemoryLoad) || (category == HW_Category_MemoryStore));
int numArgs = HWIntrinsicInfo::lookupNumArgs(tree->AsHWIntrinsic());
genConsumeAddress(tree->gtGetOp1());
if (category == HW_Category_MemoryStore)
{
assert((numArgs == 2) && !tree->gtGetOp2()->isContained());
genConsumeReg(tree->gtGetOp2());
}
else
{
assert(numArgs == 1);
}
}
else if (tree->OperIsHWIntrinsic())
#endif // FEATURE_HW_INTRINSICS
#endif // _TARGET_XARCH_
else if (tree->OperIsInitVal())
{
genConsumeReg(tree->gtGetOp1());
}
Expand Down Expand Up @@ -1368,11 +1383,6 @@ void CodeGen::genConsumeRegs(GenTree* tree)
// Return Value:
// None.
//
// Notes:
// Note that this logic is localized here because we must do the liveness update in
// the correct execution order. This is important because we may have two operands
// that involve the same lclVar, and if one is marked "lastUse" we must handle it
// after the first.

void CodeGen::genConsumeOperands(GenTreeOp* tree)
{
Expand All @@ -1389,6 +1399,55 @@ void CodeGen::genConsumeOperands(GenTreeOp* tree)
}
}

#ifdef FEATURE_HW_INTRINSICS
//------------------------------------------------------------------------
// genConsumeHWIntrinsicOperands: Do liveness update for the operands of a GT_HWIntrinsic node
//
// Arguments:
// node - the GenTreeHWIntrinsic node whose operands will have their liveness updated.
//
// Return Value:
// None.
//

void CodeGen::genConsumeHWIntrinsicOperands(GenTreeHWIntrinsic* node)
Copy link
Member

Choose a reason for hiding this comment

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

Might be nice to centralize the asserting of numArgs to number of node operands here as well?

Copy link
Author

Choose a reason for hiding this comment

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

Makes sense.

{
int numArgs = HWIntrinsicInfo::lookupNumArgs(node);
GenTree* op1 = node->gtGetOp1();
if (op1 == nullptr)
{
assert((numArgs == 0) && (node->gtGetOp2() == nullptr));
Copy link
Member

Choose a reason for hiding this comment

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

nit: It is generally nice to break && asserts into two separate ones. That makes it much clearer which assert is the problematic one when they are hit.

Copy link
Author

Choose a reason for hiding this comment

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

I generally disagree. Asserts already clutter the code, and though they are useful, they should impact the readability & flow of the code as little as possible, IMO.

Copy link

Choose a reason for hiding this comment

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

And this is another bit of code that will go away when I get rid of GT_LIST...

return;
}
if (op1->OperIs(GT_LIST))
Copy link
Member

Choose a reason for hiding this comment

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

nit: OperIsList()?

Copy link
Author

Choose a reason for hiding this comment

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

I don't think we have a convention for this, but the OperIsXXX methods pre-date @mikedn's addition of the OperIs method, which I tend to prefer since it makes it clear that you are checking for one or more specific operators. I prefer the OperIsXXX when it is used to check for a "class" of operators. cc @dotnet/jit-contrib for opinions on usage of these.

Copy link

Choose a reason for hiding this comment

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

I wouldn't bother too much with this. Hopefully we'll just get rid of GT_LIST. I have ~75% of it done, just waiting for intrinsics to finally stabilize so I don't have to keep fixing conflicts.

{
Copy link
Member

Choose a reason for hiding this comment

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

nit: assert(node->gtGetOp2() == nullptr) when op1 is a list?

Copy link
Author

Choose a reason for hiding this comment

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

Will do.

int foundArgs = 0;
assert(node->gtGetOp2() == nullptr);
for (GenTreeArgList* list = op1->AsArgList(); list != nullptr; list = list->Rest())
{
GenTree* operand = list->Current();
genConsumeRegs(operand);
foundArgs++;
}
assert(foundArgs == numArgs);
tannergooding marked this conversation as resolved.
Show resolved Hide resolved
}
else
{
genConsumeRegs(op1);
GenTree* op2 = node->gtGetOp2();
if (op2 != nullptr)
{
genConsumeRegs(op2);
assert(numArgs == 2);
}
else
{
assert(numArgs == 1);
}
}
}
#endif // FEATURE_HW_INTRINSICS

#if FEATURE_PUT_STRUCT_ARG_STK
//------------------------------------------------------------------------
// genConsumePutStructArgStk: Do liveness update for the operands of a PutArgStk node.
Expand Down
37 changes: 29 additions & 8 deletions src/jit/emitxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2867,6 +2867,12 @@ void emitter::emitInsLoadInd(instruction ins, emitAttr attr, regNumber dstReg, G
id->idReg1(dstReg);
emitHandleMemOp(mem, id, IF_RWR_ARD, ins);
UNATIVE_OFFSET sz = emitInsSizeAM(id, insCodeRM(ins));
if (Is4ByteSSEInstruction(ins))
{
// The 4-Byte SSE instructions require an additional byte.
sz += 1;
}

id->idCodeSize(sz);
dispIns(id);
emitCurIGsize += sz;
Expand Down Expand Up @@ -4055,6 +4061,12 @@ void emitter::emitIns_R_A(instruction ins, emitAttr attr, regNumber reg1, GenTre
emitHandleMemOp(indir, id, IF_RRW_ARD, ins);

UNATIVE_OFFSET sz = emitInsSizeAM(id, insCodeRM(ins));
if (Is4ByteSSEInstruction(ins))
{
// The 4-Byte SSE instructions require an additional byte.
sz += 1;
}

id->idCodeSize(sz);

dispIns(id);
Expand Down Expand Up @@ -4106,8 +4118,8 @@ void emitter::emitIns_R_AR_I(instruction ins, emitAttr attr, regNumber reg1, reg

if (Is4ByteSSEInstruction(ins))
{
// The 4-Byte SSE instructions require two additional bytes
sz += 2;
// The 4-Byte SSE instructions require an additional byte.
sz += 1;
}

id->idCodeSize(sz);
Expand Down Expand Up @@ -5183,8 +5195,8 @@ void emitter::emitIns_R_AR(instruction ins, emitAttr attr, regNumber ireg, regNu

if (Is4ByteSSEInstruction(ins))
{
// The 4-Byte SSE instructions require two additional bytes
sz += 2;
// The 4-Byte SSE instructions require an additional byte.
sz += 1;
}

id->idCodeSize(sz);
Expand Down Expand Up @@ -5658,7 +5670,7 @@ void emitter::emitIns_AX_R(instruction ins, emitAttr attr, regNumber ireg, regNu

#ifdef FEATURE_HW_INTRINSICS
//------------------------------------------------------------------------
// emitIns_SIMD_R_R_I: emits the code for a SIMD instruction that takes a register operand, an immediate operand
// 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
//
// Arguments:
Expand All @@ -5668,6 +5680,13 @@ void emitter::emitIns_AX_R(instruction ins, emitAttr attr, regNumber ireg, regNu
// op1Reg -- The register of the first operand
// ival -- The immediate value
//
// Notes:
// This will handle the required register copy if 'op1Reg' and 'targetReg' are not the same, and
// the 3-operand format is not available.
// This is not really SIMD-specific, but is currently only used in that context, as that's
// where we frequently need to handle the case of generating 3-operand or 2-operand forms
// depending on what target ISA is supported.
//
void emitter::emitIns_SIMD_R_R_I(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, int ival)
{
if (UseVEXEncoding() || IsDstSrcImmAvxInstruction(ins))
Expand Down Expand Up @@ -5722,20 +5741,22 @@ void emitter::emitIns_SIMD_R_R_A(
// targetReg -- The target register
// op1Reg -- The register of the first operand
// base -- The base register used for the memory address
// offset -- The memory offset
//
void emitter::emitIns_SIMD_R_R_AR(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, regNumber base)
void emitter::emitIns_SIMD_R_R_AR(
instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, regNumber base, int offset)
{
if (UseVEXEncoding())
{
emitIns_R_R_AR(ins, attr, targetReg, op1Reg, base, 0);
emitIns_R_R_AR(ins, attr, targetReg, op1Reg, base, offset);
}
else
{
if (op1Reg != targetReg)
{
emitIns_R_R(INS_movaps, attr, targetReg, op1Reg);
}
emitIns_R_AR(ins, attr, targetReg, base, 0);
emitIns_R_AR(ins, attr, targetReg, base, offset);
}
}

Expand Down
3 changes: 2 additions & 1 deletion src/jit/emitxarch.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,7 +449,8 @@ void emitIns_AX_R(instruction ins, emitAttr attr, regNumber ireg, regNumber reg,
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);
void emitIns_SIMD_R_R_AR(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, regNumber base);
void emitIns_SIMD_R_R_AR(
instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, regNumber base, int offset);
void emitIns_SIMD_R_R_C(
instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, CORINFO_FIELD_HANDLE fldHnd, int offs);
void emitIns_SIMD_R_R_R(instruction ins, emitAttr attr, regNumber targetReg, regNumber op1Reg, regNumber op2Reg);
Expand Down
20 changes: 20 additions & 0 deletions src/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3510,6 +3510,26 @@ unsigned Compiler::gtSetEvalOrder(GenTree* tree)
costSz = 2 * 2;
break;

#if defined(FEATURE_HW_INTRINSICS) && defined(_TARGET_XARCH_)
case GT_HWIntrinsic:
{
if (tree->AsHWIntrinsic()->OperIsMemoryLoadOrStore())
{
costEx = IND_COST_EX;
costSz = 2;
// See if we can form a complex addressing mode.

GenTree* addr = op1->gtEffectiveVal();

if (addr->OperIs(GT_ADD) && gtMarkAddrMode(addr, &costEx, &costSz, tree->TypeGet()))
{
goto DONE;
}
}
}
break;
#endif // FEATURE_HW_INTRINSICS && _TARGET_XARCH_

case GT_BLK:
case GT_IND:

Expand Down
Loading