Skip to content

Ensure Vector<T>.op_Multiply is handled as an intrinsic in appropriate cases #49503

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

Merged
tannergooding merged 9 commits into from
Mar 25, 2021
Merged

Ensure Vector<T>.op_Multiply is handled as an intrinsic in appropriate cases #49503

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
fca9b85
Ensure Vector<T>.op_Multiply is handled as an intrinsic in appropriat…
tannergooding Mar 11, 2021
24373c5
Applying formatting patch
tannergooding Mar 12, 2021
9d7c8c5
Ensure TYP_BYTE and TYP_UBYTE are handled for Vector<T>.op_Multiply o…
tannergooding Mar 12, 2021
2b5a43e
Ensure broadcast nodes are inserted for all `operator *(Vector<T>, T)`
tannergooding Mar 12, 2021
b9c77d7
Ensure ARM64 uses MultiplyByScalar when its available
tannergooding Mar 12, 2021
1b1715d
Applying formatting patch
tannergooding Mar 12, 2021
a79a1cb
Ensure the scalar for op_Multiply is op2 on ARM64
tannergooding Mar 15, 2021
56c210d
Ensure we do a full multiply for `Vector<T> * Vector<T>` on ARM64
tannergooding Mar 15, 2021
2d72fd0
Applying formatting patch
tannergooding Mar 17, 2021
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
241 changes: 208 additions & 33 deletions src/coreclr/jit/simdashwintrinsic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -880,56 +880,154 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,

case NI_VectorT128_op_Multiply:
{
assert(baseType == TYP_INT);

NamedIntrinsic hwIntrinsic = NI_Illegal;
GenTree** broadcastOp = nullptr;

if (compOpportunisticallyDependsOn(InstructionSet_SSE41))
if (varTypeIsArithmetic(op1->TypeGet()))
{
hwIntrinsic = NI_SSE41_MultiplyLow;
broadcastOp = &op1;
}
else
else if (varTypeIsArithmetic(op2->TypeGet()))
{
// op1Dup = op1
GenTree* op1Dup;
op1 = impCloneExpr(op1, &op1Dup, clsHnd, (unsigned)CHECK_SPILL_ALL,
nullptr DEBUGARG("Clone op1 for Vector<T>.Multiply"));
broadcastOp = &op2;
}

// op2Dup = op2
GenTree* op2Dup;
op2 = impCloneExpr(op2, &op2Dup, clsHnd, (unsigned)CHECK_SPILL_ALL,
nullptr DEBUGARG("Clone op2 for Vector<T>.Multiply"));
if (broadcastOp != nullptr)
{
*broadcastOp = gtNewSimdCreateBroadcastNode(simdType, *broadcastOp, baseType, simdSize,
/* isSimdAsHWIntrinsic */ true);
}

// op1 = Sse2.ShiftRightLogical128BitLane(op1, 4)
op1 = gtNewSimdAsHWIntrinsicNode(retType, op1, gtNewIconNode(4, TYP_INT),
NI_SSE2_ShiftRightLogical128BitLane, baseType, simdSize);
switch (baseType)
{
case TYP_SHORT:
case TYP_USHORT:
{
hwIntrinsic = NI_SSE2_MultiplyLow;
break;
}

// op2 = Sse2.ShiftRightLogical128BitLane(op1, 4)
op2 = gtNewSimdAsHWIntrinsicNode(retType, op2, gtNewIconNode(4, TYP_INT),
NI_SSE2_ShiftRightLogical128BitLane, baseType, simdSize);
case TYP_INT:
case TYP_UINT:
{
if (compOpportunisticallyDependsOn(InstructionSet_SSE41))
{
hwIntrinsic = NI_SSE41_MultiplyLow;
}
else
{
// op1Dup = op1
GenTree* op1Dup;
op1 = impCloneExpr(op1, &op1Dup, clsHnd, (unsigned)CHECK_SPILL_ALL,
nullptr DEBUGARG("Clone op1 for Vector<T>.Multiply"));

// op2Dup = op2
GenTree* op2Dup;
op2 = impCloneExpr(op2, &op2Dup, clsHnd, (unsigned)CHECK_SPILL_ALL,
nullptr DEBUGARG("Clone op2 for Vector<T>.Multiply"));

// op1 = Sse2.ShiftRightLogical128BitLane(op1, 4)
op1 =
gtNewSimdAsHWIntrinsicNode(retType, op1, gtNewIconNode(4, TYP_INT),
NI_SSE2_ShiftRightLogical128BitLane, baseType, simdSize);

// op2 = Sse2.ShiftRightLogical128BitLane(op1, 4)
op2 =
gtNewSimdAsHWIntrinsicNode(retType, op2, gtNewIconNode(4, TYP_INT),
NI_SSE2_ShiftRightLogical128BitLane, baseType, simdSize);

// op2 = Sse2.Multiply(op2.AsUInt64(), op1.AsUInt64()).AsInt32()
op2 = gtNewSimdAsHWIntrinsicNode(retType, op2, op1, NI_SSE2_Multiply, TYP_ULONG,
simdSize);

// op2 = Sse2.Shuffle(op2, (0, 0, 2, 0))
op2 = gtNewSimdAsHWIntrinsicNode(retType, op2, gtNewIconNode(SHUFFLE_XXZX, TYP_INT),
NI_SSE2_Shuffle, baseType, simdSize);

// op1 = Sse2.Multiply(op1Dup.AsUInt64(), op2Dup.AsUInt64()).AsInt32()
op1 = gtNewSimdAsHWIntrinsicNode(retType, op1Dup, op2Dup, NI_SSE2_Multiply, TYP_ULONG,
simdSize);

// op1 = Sse2.Shuffle(op1, (0, 0, 2, 0))
op1 = gtNewSimdAsHWIntrinsicNode(retType, op1, gtNewIconNode(SHUFFLE_XXZX, TYP_INT),
NI_SSE2_Shuffle, baseType, simdSize);

// result = Sse2.UnpackLow(op1, op2)
hwIntrinsic = NI_SSE2_UnpackLow;
}
break;
}

// op2 = Sse2.Multiply(op2.AsUInt64(), op1.AsUInt64()).AsInt32()
op2 = gtNewSimdAsHWIntrinsicNode(retType, op2, op1, NI_SSE2_Multiply, TYP_ULONG, simdSize);
case TYP_FLOAT:
{
hwIntrinsic = NI_SSE_Multiply;
break;
}

// op2 = Sse2.Shuffle(op2, (0, 0, 2, 0))
op2 = gtNewSimdAsHWIntrinsicNode(retType, op2, gtNewIconNode(SHUFFLE_XXZX, TYP_INT),
NI_SSE2_Shuffle, baseType, simdSize);
case TYP_DOUBLE:
{
hwIntrinsic = NI_SSE2_Multiply;
break;
}

// op1 = Sse2.Multiply(op1Dup.AsUInt64(), op2Dup.AsUInt64()).AsInt32()
op1 =
gtNewSimdAsHWIntrinsicNode(retType, op1Dup, op2Dup, NI_SSE2_Multiply, TYP_ULONG, simdSize);
default:
{
unreached();
}
}

assert(hwIntrinsic != NI_Illegal);
return gtNewSimdAsHWIntrinsicNode(retType, op1, op2, hwIntrinsic, baseType, simdSize);
}

// op1 = Sse2.Shuffle(op1, (0, 0, 2, 0))
op1 = gtNewSimdAsHWIntrinsicNode(retType, op1, gtNewIconNode(SHUFFLE_XXZX, TYP_INT),
NI_SSE2_Shuffle, baseType, simdSize);
case NI_VectorT256_op_Multiply:
{
NamedIntrinsic hwIntrinsic = NI_Illegal;
GenTree** broadcastOp = nullptr;

// result = Sse2.UnpackLow(op1, op2)
hwIntrinsic = NI_SSE2_UnpackLow;
if (varTypeIsArithmetic(op1->TypeGet()))
{
broadcastOp = &op1;
}
else if (varTypeIsArithmetic(op2->TypeGet()))
{
broadcastOp = &op2;
}
assert(hwIntrinsic != NI_Illegal);

if (broadcastOp != nullptr)
{
*broadcastOp = gtNewSimdCreateBroadcastNode(simdType, *broadcastOp, baseType, simdSize,
/* isSimdAsHWIntrinsic */ true);
}

switch (baseType)
{
case TYP_SHORT:
case TYP_USHORT:
case TYP_INT:
case TYP_UINT:
{
hwIntrinsic = NI_AVX2_MultiplyLow;

@echesakov echesakov Mar 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouildn't this be guarded with compOpportunisticallyDependsOn(InstructionSet_AVX2)?

@tannergooding tannergooding Mar 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

No, its already asserted at the top of the method (https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/simdashwintrinsic.cpp#L392) as well as properly guarded further up in the stack.

We should never encounter SimdAsHWIntrinsicClassId::VectorT256 if AVX2 is not supported.

@echesakov echesakov Mar 13, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why at least AVX2 is required for VectorT256? Shouldn't AVX be sufficient?

@tannergooding tannergooding Mar 13, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Vector<T> supports all 10 primitive types and can only properly accelerate the integer types with AVX2 so it was decided it is only 32-bytes if AVX2 is supported and is 16-bytes otherwise, for the best overall perf/experience.

break;
}

case TYP_FLOAT:
case TYP_DOUBLE:
{
hwIntrinsic = NI_AVX_Multiply;
break;
}

default:
{
unreached();
}
}

assert(hwIntrinsic != NI_Illegal);
return gtNewSimdAsHWIntrinsicNode(retType, op1, op2, hwIntrinsic, baseType, simdSize);
}

#elif defined(TARGET_ARM64)
case NI_Vector2_CreateBroadcast:
case NI_Vector3_CreateBroadcast:
Expand Down Expand Up @@ -970,6 +1068,83 @@ GenTree* Compiler::impSimdAsHWIntrinsicSpecial(NamedIntrinsic intrinsic,
// result = ConditionalSelect(op1, op1Dup, op2Dup)
return impSimdAsHWIntrinsicCndSel(clsHnd, retType, baseType, simdSize, op1, op1Dup, op2Dup);
}

case NI_VectorT128_op_Multiply:
{
NamedIntrinsic hwIntrinsic = NI_Illegal;
NamedIntrinsic scalarIntrinsic = NI_Illegal;
GenTree** scalarOp = nullptr;

if (varTypeIsArithmetic(op1->TypeGet()))
{

@echesakov echesakov Mar 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

For Arm64 you don't neet broadcast operation when either op1 or op2 are floating-point.
Since the operands will be in SIMD registers already you should use MultiplyByElement (where elementIndex is 0) instead

@echesakov echesakov Mar 12, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I also don't think you need to broadcast an integer value to the whole SIMD register.
You can use ins Vd.T[0], Xd followed by mul Vd, Vn, Vd.T[0] instead.

In this case, you can share some of the implementation for floating-point and integer types.

@tannergooding tannergooding Mar 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We should need a CreateScalarUnsafe in this case instead then. I'll update to do the right thing.

  • Notably this wasn't possible before the ARM HWIntrinsics were brought online, which is likely why Vector wasn't doing that already.

@tannergooding tannergooding Mar 12, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Notably, MultiplyByScalar and MultiplyBySelectedScalar don't work for byte/sbyte and so those still need a broadcast.

// MultiplyByScalar requires the scalar op to be op2
std::swap(op1, op2);

scalarOp = &op2;
}
else if (varTypeIsArithmetic(op2->TypeGet()))
{
scalarOp = &op2;
}

switch (baseType)
{
case TYP_BYTE:
case TYP_UBYTE:
{
if (scalarOp != nullptr)
{
*scalarOp = gtNewSimdCreateBroadcastNode(simdType, *scalarOp, baseType, simdSize,
/* isSimdAsHWIntrinsic */ true);
}

hwIntrinsic = NI_AdvSimd_Multiply;
break;
}

case TYP_SHORT:
case TYP_USHORT:
case TYP_INT:
case TYP_UINT:
case TYP_FLOAT:
{
if (scalarOp != nullptr)
{
hwIntrinsic = NI_AdvSimd_MultiplyByScalar;
*scalarOp = gtNewSimdAsHWIntrinsicNode(TYP_SIMD8, *scalarOp,
NI_Vector64_CreateScalarUnsafe, baseType, 8);
}
else
{
hwIntrinsic = NI_AdvSimd_Multiply;
}
break;
}

case TYP_DOUBLE:
{
if (scalarOp != nullptr)
{
hwIntrinsic = NI_AdvSimd_Arm64_MultiplyByScalar;
*scalarOp =
gtNewSimdAsHWIntrinsicNode(TYP_SIMD8, *scalarOp, NI_Vector64_Create, baseType, 8);
}
else
{
hwIntrinsic = NI_AdvSimd_Arm64_Multiply;
}
break;
}

default:
{
unreached();
}
}

assert(hwIntrinsic != NI_Illegal);
return gtNewSimdAsHWIntrinsicNode(retType, op1, op2, hwIntrinsic, baseType, simdSize);
}
#else
#error Unsupported platform
#endif // !TARGET_XARCH && !TARGET_ARM64
Expand Down
2 changes: 1 addition & 1 deletion src/coreclr/jit/simdashwintrinsiclistarm64.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,7 @@ SIMD_AS_HWINTRINSIC_ID(VectorT128, op_Equality,
SIMD_AS_HWINTRINSIC_ID(VectorT128, op_ExclusiveOr, 2, {NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor, NI_AdvSimd_Xor}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT128, op_Explicit, 1, {NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit, NI_VectorT128_op_Explicit}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT128, op_Inequality, 2, {NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality, NI_Vector128_op_Inequality}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT128, op_Multiply, 2, {NI_AdvSimd_Multiply, NI_AdvSimd_Multiply, NI_AdvSimd_Multiply, NI_AdvSimd_Multiply, NI_AdvSimd_Multiply, NI_AdvSimd_Multiply, NI_Illegal, NI_Illegal, NI_AdvSimd_Multiply, NI_AdvSimd_Arm64_Multiply}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT128, op_Multiply, 2, {NI_VectorT128_op_Multiply, NI_VectorT128_op_Multiply, NI_VectorT128_op_Multiply, NI_VectorT128_op_Multiply, NI_VectorT128_op_Multiply, NI_VectorT128_op_Multiply, NI_Illegal, NI_Illegal, NI_VectorT128_op_Multiply, NI_VectorT128_op_Multiply}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT128, op_Subtraction, 2, {NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Subtract, NI_AdvSimd_Arm64_Subtract}, SimdAsHWIntrinsicFlag::None)
SIMD_AS_HWINTRINSIC_ID(VectorT128, SquareRoot, 1, {NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_Illegal, NI_AdvSimd_Arm64_Sqrt, NI_AdvSimd_Arm64_Sqrt}, SimdAsHWIntrinsicFlag::None)

Expand Down
Loading