-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
Adding basic support for recognizing and handling SIMD intrinsics as HW intrinsics #35421
Changes from 31 commits
703ab93
32209f3
a5ad01c
3b5f8f4
8744b7b
4beacab
3af99b2
e229ca0
92ec83c
e9e7b89
f788049
0cf2a0b
10e7235
9022d94
16048cd
82d646e
ed399c4
1e57f6e
aaa9962
d22ecab
5830101
4f02f57
39f5086
fe969fd
bf291c5
ef4a77c
e1b5acb
1b837a6
ac867ec
7482c49
c7ef80d
e569d25
991a838
017fe54
06bec3e
bd6e87a
341aac8
b6494ee
470f627
03840fa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -745,7 +745,7 @@ struct GenTree | |
|
||
#define GTF_UNSIGNED 0x00008000 // With GT_CAST: the source operand is an unsigned type | ||
// With operators: the specified node is an unsigned operator | ||
// | ||
// | ||
#define GTF_LATE_ARG 0x00010000 // The specified node is evaluated to a temp in the arg list, and this temp is added to gtCallLateArgs. | ||
#define GTF_SPILL 0x00020000 // Needs to be spilled here | ||
|
||
|
@@ -913,6 +913,9 @@ struct GenTree | |
#define GTF_SIMD12_OP 0x80000000 // GT_SIMD -- Indicates that the operands need to be handled as SIMD12 | ||
// even if they have been retyped as SIMD16. | ||
|
||
#define GTF_SIMDASHW_OP 0x80000000 // GT_HWINTRINSIC -- Indicates that the structHandle should be gotten from gtGetStructHandleForSIMD | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I was looking at resolving the struct handle issue (called out #35421 (comment)) differently at first. As far as the JIT needs to be concerned, there isn't really a difference between two However, recognizing this is non-trivial and so I went with the simpler approach of just flagging the operation as taking/returning Unlike |
||
// rarther than from gtGetStructHandleForHWSIMD. | ||
|
||
//--------------------------------------------------------------------- | ||
// | ||
// GenTree flags stored in gtDebugFlags. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -308,7 +308,7 @@ struct HWIntrinsicInfo | |
} | ||
|
||
#ifdef TARGET_XARCH | ||
static int lookupIval(NamedIntrinsic id) | ||
static int lookupIval(NamedIntrinsic id, bool opportunisticallyDependsOnAVX) | ||
{ | ||
switch (id) | ||
{ | ||
|
@@ -325,6 +325,17 @@ struct HWIntrinsicInfo | |
case NI_SSE_CompareScalarGreaterThan: | ||
case NI_SSE2_CompareGreaterThan: | ||
case NI_SSE2_CompareScalarGreaterThan: | ||
case NI_AVX_CompareGreaterThan: | ||
{ | ||
if (opportunisticallyDependsOnAVX) | ||
{ | ||
return static_cast<int>(FloatComparisonMode::OrderedGreaterThanSignaling); | ||
} | ||
|
||
assert(id != NI_AVX_CompareGreaterThan); | ||
return static_cast<int>(FloatComparisonMode::OrderedLessThanSignaling); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This could be confusing to someone who doesn't know that we expect later in the JIT to swap the intrinsic arguments. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. IMO, it would be clearer to leave these as special import intrinsics and move all the plumbing related to opportunisticallyDependsOnAVX to one place. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can add a comment, but the entire point of moving it to lowering is so the rest of the JIT doesn't need to care that As we continue adding more JIT optimizations around HWIntrinsics, the distinction doesn't matter to anything except for codegen and so handling the fixup in lowering makes this trivial for everything else. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Will submit a follow up PR that adds the comment. |
||
} | ||
|
||
case NI_SSE_CompareLessThan: | ||
case NI_SSE_CompareScalarLessThan: | ||
case NI_SSE2_CompareLessThan: | ||
|
@@ -338,6 +349,17 @@ struct HWIntrinsicInfo | |
case NI_SSE_CompareScalarGreaterThanOrEqual: | ||
case NI_SSE2_CompareGreaterThanOrEqual: | ||
case NI_SSE2_CompareScalarGreaterThanOrEqual: | ||
case NI_AVX_CompareGreaterThanOrEqual: | ||
{ | ||
if (opportunisticallyDependsOnAVX) | ||
{ | ||
return static_cast<int>(FloatComparisonMode::OrderedGreaterThanOrEqualSignaling); | ||
} | ||
|
||
assert(id != NI_AVX_CompareGreaterThanOrEqual); | ||
return static_cast<int>(FloatComparisonMode::OrderedLessThanOrEqualSignaling); | ||
} | ||
|
||
case NI_SSE_CompareLessThanOrEqual: | ||
case NI_SSE_CompareScalarLessThanOrEqual: | ||
case NI_SSE2_CompareLessThanOrEqual: | ||
|
@@ -360,6 +382,17 @@ struct HWIntrinsicInfo | |
case NI_SSE_CompareScalarNotGreaterThan: | ||
case NI_SSE2_CompareNotGreaterThan: | ||
case NI_SSE2_CompareScalarNotGreaterThan: | ||
case NI_AVX_CompareNotGreaterThan: | ||
{ | ||
if (opportunisticallyDependsOnAVX) | ||
{ | ||
return static_cast<int>(FloatComparisonMode::UnorderedNotGreaterThanSignaling); | ||
} | ||
|
||
assert(id != NI_AVX_CompareNotGreaterThan); | ||
return static_cast<int>(FloatComparisonMode::UnorderedNotLessThanSignaling); | ||
} | ||
|
||
case NI_SSE_CompareNotLessThan: | ||
case NI_SSE_CompareScalarNotLessThan: | ||
case NI_SSE2_CompareNotLessThan: | ||
|
@@ -373,6 +406,17 @@ struct HWIntrinsicInfo | |
case NI_SSE_CompareScalarNotGreaterThanOrEqual: | ||
case NI_SSE2_CompareNotGreaterThanOrEqual: | ||
case NI_SSE2_CompareScalarNotGreaterThanOrEqual: | ||
case NI_AVX_CompareNotGreaterThanOrEqual: | ||
{ | ||
if (opportunisticallyDependsOnAVX) | ||
{ | ||
return static_cast<int>(FloatComparisonMode::UnorderedNotGreaterThanOrEqualSignaling); | ||
} | ||
|
||
assert(id != NI_AVX_CompareNotGreaterThanOrEqual); | ||
return static_cast<int>(FloatComparisonMode::UnorderedNotLessThanOrEqualSignaling); | ||
} | ||
|
||
case NI_SSE_CompareNotLessThanOrEqual: | ||
case NI_SSE_CompareScalarNotLessThanOrEqual: | ||
case NI_SSE2_CompareNotLessThanOrEqual: | ||
|
@@ -441,26 +485,6 @@ struct HWIntrinsicInfo | |
return static_cast<int>(FloatRoundingMode::ToZero); | ||
} | ||
|
||
case NI_AVX_CompareGreaterThan: | ||
{ | ||
return static_cast<int>(FloatComparisonMode::OrderedGreaterThanSignaling); | ||
} | ||
|
||
case NI_AVX_CompareGreaterThanOrEqual: | ||
{ | ||
return static_cast<int>(FloatComparisonMode::OrderedGreaterThanOrEqualSignaling); | ||
} | ||
|
||
case NI_AVX_CompareNotGreaterThan: | ||
{ | ||
return static_cast<int>(FloatComparisonMode::UnorderedNotGreaterThanSignaling); | ||
} | ||
|
||
case NI_AVX_CompareNotGreaterThanOrEqual: | ||
{ | ||
return static_cast<int>(FloatComparisonMode::UnorderedNotGreaterThanOrEqualSignaling); | ||
} | ||
|
||
default: | ||
{ | ||
return -1; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,7 +207,7 @@ void CodeGen::genHWIntrinsic(GenTreeHWIntrinsic* node) | |
} | ||
else | ||
{ | ||
emitSize = EA_SIZE(node->gtSIMDSize); | ||
emitSize = emitActualTypeSize(Compiler::getSIMDTypeForSize(node->gtSIMDSize)); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this needed? To support Vector3 on Arm64? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yes, its required to support |
||
opt = genGetSimdInsOpt(emitSize, intrin.baseType); | ||
|
||
if ((opt == INS_OPTS_1D) && (intrin.category == HW_Category_SimpleSIMD)) | ||
|
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.
We could also change
GenTreeHWIntrinsic
to track the SimdType rather than the SimdSize, but it is a more involved change.