Implement AVX2 Gather intrinsic #19392
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -479,8 +479,8 @@ struct GenTree | |
| // happening. | ||
| void CopyCosts(const GenTree* const tree) | ||
| { | ||
| // If the 'tree' costs aren't initialized, we'll hit an assert below. | ||
|
There was a problem hiding this comment. Why are all these random comment re-alignments popping up? They seem unrelated to the PR. There was a problem hiding this comment. Clang-format is confused by some comments, so I fixed here to avoid more unrelated changes from clang-format. |
||
| INDEBUG(gtCostsInitialized = tree->gtCostsInitialized;) | ||
| _gtCostEx = tree->gtCostEx; | ||
| _gtCostSz = tree->gtCostSz; | ||
| } | ||
|
|
@@ -4115,6 +4115,7 @@ struct GenTreeSIMD : public GenTreeJitIntrinsic | |
| struct GenTreeHWIntrinsic : public GenTreeJitIntrinsic | ||
| { | ||
| NamedIntrinsic gtHWIntrinsicId; | ||
| var_types gtIndexBaseType; // for AVX2 Gather* intrinsics | ||
|
There was a problem hiding this comment. Gather intrinsics have complex overloads that need additional information (the base-type of index vector) for codegen, so adding a field in IR. But that let GenTreeHWIntrinsic become a large node. @CarolEidt do you think it ok? There was a problem hiding this comment. It is probably not a big issue for intrinsics, though for methods with heavy intrinsic usage it could be an impact. Did you consider deriving from There was a problem hiding this comment. It might also be worth considering whether to define something like There was a problem hiding this comment. Perhaps make There are also spare bytes in the There was a problem hiding this comment.
@mikedn Good point! Yes, 64k is definitely enough for the foreseeable future (AVX-512 based ISAs). There was a problem hiding this comment.
Made this change, now There was a problem hiding this comment. What is the |
||
|
|
||
| GenTreeHWIntrinsic(var_types type, NamedIntrinsic hwIntrinsicID, var_types baseType, unsigned size) | ||
| : GenTreeJitIntrinsic(GT_HWIntrinsic, type, nullptr, nullptr, baseType, size), gtHWIntrinsicId(hwIntrinsicID) | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1184,6 +1184,9 @@ void CodeGen::genHWIntrinsicJumpTableFallback(NamedIntrinsic intrinsi | |
| HWIntrinsicSwitchCaseBody emitSwCase) | ||
| { | ||
| assert(nonConstImmReg != REG_NA); | ||
| // AVX2 Gather intrinsics use managed non-const fallback since they have discrete imm8 value range | ||
| // that does work with the current compiler generated jump-table fallback | ||
| assert(!HWIntrinsicInfo::isAVX2GatherIntrinsic(intrinsic)); | ||
| emitter* emit = getEmitter(); | ||
|
|
||
| const unsigned maxByte = (unsigned)HWIntrinsicInfo::lookupImmUpperBound(intrinsic) + 1; | ||
|
|
@@ -2008,6 +2011,117 @@ void CodeGen::genAvxOrAvx2Intrinsic(GenTreeHWIntrinsic* node) | |
| break; | ||
| } | ||
|
|
||
| case NI_AVX2_GatherVector128: | ||
| case NI_AVX2_GatherVector256: | ||
| case NI_AVX2_GatherMaskVector128: | ||
| case NI_AVX2_GatherMaskVector256: | ||
| { | ||
| GenTreeArgList* list = op1->AsArgList(); | ||
|
There was a problem hiding this comment. nit: It is helpful to break up the argList blocks, for readability: Ex: coreclr/src/jit/hwintrinsiccodegenxarch.cpp Line 217 in 7f36c93 |
||
| op1 = list->Current(); | ||
| op1Reg = op1->gtRegNum; | ||
| genConsumeRegs(op1); | ||
|
|
||
| list = list->Rest(); | ||
| op2 = list->Current(); | ||
| op2Reg = op2->gtRegNum; | ||
| genConsumeRegs(op2); | ||
|
|
||
| list = list->Rest(); | ||
| GenTree* op3 = list->Current(); | ||
| genConsumeRegs(op3); | ||
|
|
||
| list = list->Rest(); | ||
| GenTree* op4 = nullptr; | ||
| GenTree* lastOp = nullptr; | ||
| GenTree* indexOp = nullptr; | ||
|
|
||
| regNumber op3Reg = REG_NA; | ||
| regNumber op4Reg = REG_NA; | ||
| regNumber addrBaseReg = REG_NA; | ||
| regNumber addrIndexReg = REG_NA; | ||
| regNumber maskReg = node->ExtractTempReg(RBM_ALLFLOAT); | ||
|
|
||
| if (numArgs == 5) | ||
| { | ||
| assert(intrinsicId == NI_AVX2_GatherMaskVector128 || intrinsicId == NI_AVX2_GatherMaskVector256); | ||
| op4 = list->Current(); | ||
| list = list->Rest(); | ||
| lastOp = list->Current(); | ||
| op3Reg = op3->gtRegNum; | ||
| op4Reg = op4->gtRegNum; | ||
| genConsumeRegs(op4); | ||
| addrBaseReg = op2Reg; | ||
| addrIndexReg = op3Reg; | ||
| indexOp = op3; | ||
|
|
||
| // copy op4Reg into the tmp mask register, | ||
| // the mask register will be cleared by gather instructions | ||
| emit->emitIns_R_R(INS_movaps, attr, maskReg, op4Reg); | ||
|
|
||
| if (targetReg != op1Reg) | ||
| { | ||
| // copy source vector to the target register for masking merge | ||
| emit->emitIns_R_R(INS_movaps, attr, targetReg, op1Reg); | ||
|
There was a problem hiding this comment. What if There was a problem hiding this comment. Op2Reg is a GPR (base register of the address) and cannot be same as the vector target register. There was a problem hiding this comment. A comment clarifying this would be nice |
||
| } | ||
| } | ||
| else | ||
| { | ||
| assert(intrinsicId == NI_AVX2_GatherVector128 || intrinsicId == NI_AVX2_GatherVector256); | ||
| addrBaseReg = op1Reg; | ||
| addrIndexReg = op2Reg; | ||
| indexOp = op2; | ||
| lastOp = op3; | ||
|
|
||
| // generate all-one mask vector | ||
| emit->emitIns_SIMD_R_R_R(INS_pcmpeqd, attr, maskReg, maskReg, maskReg); | ||
| } | ||
|
|
||
| bool isVector128GatherWithVector256Index = (targetType == TYP_SIMD16) && (indexOp->TypeGet() == TYP_SIMD32); | ||
|
|
||
| // hwintrinsiclistxarch.h uses Dword index instructions in default | ||
| if (varTypeIsLong(node->gtIndexBaseType)) | ||
| { | ||
| switch (ins) | ||
| { | ||
| case INS_vpgatherdd: | ||
| ins = INS_vpgatherqd; | ||
| if (isVector128GatherWithVector256Index) | ||
| { | ||
| // YMM index in address mode | ||
| attr = emitTypeSize(TYP_SIMD32); | ||
| } | ||
| break; | ||
| case INS_vpgatherdq: | ||
| ins = INS_vpgatherqq; | ||
| break; | ||
| case INS_vgatherdps: | ||
| ins = INS_vgatherqps; | ||
| if (isVector128GatherWithVector256Index) | ||
| { | ||
| // YMM index in address mode | ||
| attr = emitTypeSize(TYP_SIMD32); | ||
| } | ||
| break; | ||
| case INS_vgatherdpd: | ||
| ins = INS_vgatherqpd; | ||
| break; | ||
| default: | ||
| unreached(); | ||
| } | ||
| } | ||
|
|
||
| assert(lastOp->IsCnsIntOrI()); | ||
| ssize_t ival = lastOp->AsIntCon()->IconValue(); | ||
| assert((ival >= 0) && (ival <= 255)); | ||
|
|
||
| assert(targetReg != maskReg); | ||
| assert(targetReg != addrIndexReg); | ||
| assert(maskReg != addrIndexReg); | ||
| emit->emitIns_R_AR_R(ins, attr, targetReg, maskReg, addrBaseReg, addrIndexReg, (int8_t)ival, 0); | ||
|
|
||
| break; | ||
| } | ||
|
|
||
| case NI_AVX_GetLowerHalf: | ||
| { | ||
| assert(op2 == nullptr); | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we shouldn't be encoding the scale/offs in an
Indirnode and lettingemitHandleMemOphandle it@CarolEidt ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The scale/offs would be on a
GenTreeAddrMode- presumably aGT_LEA. It would make sense for that to be a child of the gather node, which would not only consolidate the scale/offs handling, but also would reduce the operand count of the gather.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@CarolEidt Thanks for the comments. Do you suggest to make the change in this PR? Or I can improve it in a new PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it would be fine (and probably a bit cleaner) to do it as a separate PR.