Skip to content

Upgrading Vector256/512 Shuffle() with VBMI support #87083

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
BruceForstall merged 4 commits into from
Jun 12, 2023
Merged

Upgrading Vector256/512 Shuffle() with VBMI support #87083

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
11ae386
Upgrading Shuffle()
DeepakRajendrakumaran May 31, 2023
4832e6e
Simplifying 'needsZero' initialization. Should be initialized to 'fal…
DeepakRajendrakumaran Jun 5, 2023
272b5ac
Review Changes
DeepakRajendrakumaran Jun 6, 2023
17fb7dc
Another review fix.
DeepakRajendrakumaran Jun 9, 2023
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
40 changes: 38 additions & 2 deletions src/coreclr/jit/gentree.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23322,7 +23322,7 @@ GenTree* Compiler::gtNewSimdShuffleNode(
#if defined(TARGET_XARCH)
uint8_t control = 0;
bool crossLane = false;
bool needsZero = varTypeIsSmallInt(simdBaseType) && (simdSize != 64);
bool needsZero = varTypeIsSmallInt(simdBaseType) && (simdSize <= 16);
Comment thread
tannergooding marked this conversation as resolved.
Resolved
uint64_t value = 0;
simd_t vecCns = {};
simd_t mskCns = {};
Expand Down Expand Up @@ -23395,7 +23395,8 @@ GenTree* Compiler::gtNewSimdShuffleNode(
{
assert(compIsaSupportedDebugOnly(InstructionSet_AVX2));

if (varTypeIsSmallInt(simdBaseType))
if ((varTypeIsByte(simdBaseType) && !compOpportunisticallyDependsOn(InstructionSet_AVX512VBMI_VL)) ||
(varTypeIsShort(simdBaseType) && !compOpportunisticallyDependsOn(InstructionSet_AVX512BW_VL)))
{
if (crossLane)
{
Expand Down Expand Up @@ -23448,6 +23449,31 @@ GenTree* Compiler::gtNewSimdShuffleNode(
// swap the operands to match the encoding requirements
retNode = gtNewSimdHWIntrinsicNode(type, op2, op1, NI_AVX2_PermuteVar8x32, simdBaseJitType, simdSize);
}
else if (elementSize == 2)
{
Comment thread
tannergooding marked this conversation as resolved.
Resolved
assert(compIsaSupportedDebugOnly(InstructionSet_AVX512BW_VL));
for (uint32_t i = 0; i < elementCount; i++)
{
vecCns.u16[i] = (uint8_t)(vecCns.u8[i * elementSize] / elementSize);
}

op2 = gtNewVconNode(type);
op2->AsVecCon()->gtSimdVal = vecCns;

// swap the operands to match the encoding requirements
retNode =
gtNewSimdHWIntrinsicNode(type, op2, op1, NI_AVX512BW_VL_PermuteVar16x16, simdBaseJitType, simdSize);
}
else if (elementSize == 1)
{
assert(compIsaSupportedDebugOnly(InstructionSet_AVX512VBMI_VL));
op2 = gtNewVconNode(type);
op2->AsVecCon()->gtSimdVal = vecCns;

// swap the operands to match the encoding requirements
retNode =
gtNewSimdHWIntrinsicNode(type, op2, op1, NI_AVX512VBMI_VL_PermuteVar32x8, simdBaseJitType, simdSize);
}
else
{
assert(elementSize == 8);
Expand All @@ -23458,6 +23484,7 @@ GenTree* Compiler::gtNewSimdShuffleNode(
}
else if (simdSize == 64)
{
assert(IsBaselineVector512IsaSupportedDebugOnly());
if (elementSize == 4)
{
for (uint32_t i = 0; i < elementCount; i++)
Expand All @@ -23484,6 +23511,15 @@ GenTree* Compiler::gtNewSimdShuffleNode(
// swap the operands to match the encoding requirements
retNode = gtNewSimdHWIntrinsicNode(type, op2, op1, NI_AVX512BW_PermuteVar32x16, simdBaseJitType, simdSize);
}
else if (elementSize == 1)

@tannergooding tannergooding Jun 9, 2023

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

There's potentially a missing "todo" here to handle this using pshufb if we don't cross lanes and VBMI is not available.

I don't think it needs to be handled as part of this PR (or even for .NET 8 necessarily) since that scenario should only be limited to pre-cannon lake (and therefore first generation AVX-512 hardware). But, since it should just be refactoring the logic under the if (simdSize == 32) path to be shared; its probably worth having still.

{
assert(compIsaSupportedDebugOnly(InstructionSet_AVX512VBMI));
op2 = gtNewVconNode(type);
op2->AsVecCon()->gtSimdVal = vecCns;

// swap the operands to match the encoding requirements
retNode = gtNewSimdHWIntrinsicNode(type, op2, op1, NI_AVX512VBMI_PermuteVar64x8, simdBaseJitType, simdSize);
}
Comment thread
tannergooding marked this conversation as resolved.
Resolved
else
{
assert(elementSize == 8);
Expand Down
10 changes: 6 additions & 4 deletions src/coreclr/jit/hwintrinsicxarch.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2620,13 +2620,15 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,

if (simdSize == 32)
{
if (!compExactlyDependsOn(InstructionSet_AVX2))
if (!compOpportunisticallyDependsOn(InstructionSet_AVX2))
{
// While we could accelerate some functions on hardware with only AVX support
// it's likely not worth it overall given that IsHardwareAccelerated reports false
break;
}
else if (varTypeIsSmallInt(simdBaseType))
else if ((varTypeIsByte(simdBaseType) &&
!compOpportunisticallyDependsOn(InstructionSet_AVX512VBMI_VL)) ||
(varTypeIsShort(simdBaseType) && !compOpportunisticallyDependsOn(InstructionSet_AVX512BW_VL)))
{
bool crossLane = false;

Expand Down Expand Up @@ -2663,7 +2665,7 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
}
else if (simdSize == 64)
{
if (varTypeIsByte(simdBaseType))
if (varTypeIsByte(simdBaseType) && !compOpportunisticallyDependsOn(InstructionSet_AVX512VBMI))
{
// TYP_BYTE, TYP_UBYTE need AVX512VBMI.
break;
Expand All @@ -2673,7 +2675,7 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
{
assert(simdSize == 16);

if (varTypeIsSmallInt(simdBaseType) && !compExactlyDependsOn(InstructionSet_SSSE3))
if (varTypeIsSmallInt(simdBaseType) && !compOpportunisticallyDependsOn(InstructionSet_SSSE3))
{
// TYP_BYTE, TYP_UBYTE, TYP_SHORT, and TYP_USHORT need SSSE3 to be able to shuffle any operation
break;
Expand Down