Skip to content

Commit f9d38d5

Browse files
tannergoodingtannergooding
committed
Ensure that constant folding for SIMD shifts on xarch follow the correct behavior on overshift (dotnet#98001)
* Ensure that constant folding for SIMD shifts on xarch follow the correct behavior on overshift * Ensure we test Sse2.IsSupported
1 parent e6d8079 commit f9d38d5

5 files changed

Lines changed: 184 additions & 4 deletions

File tree

‎src/coreclr/jit/hwintrinsicxarch.cpp‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -924,7 +924,7 @@ GenTree* Compiler::impNonConstFallback(NamedIntrinsic intrinsic, var_types simdT
924924
GenTree* op2 = impPopStack().val;
925925
GenTree* op1 = impSIMDPopStack();
926926

927-
GenTree* tmpOp = gtNewSimdCreateScalarUnsafeNode(TYP_SIMD16, op2, CORINFO_TYPE_INT, 16);
927+
GenTree* tmpOp = gtNewSimdCreateScalarNode(TYP_SIMD16, op2, CORINFO_TYPE_INT, 16);
928928
return gtNewSimdHWIntrinsicNode(simdType, op1, tmpOp, intrinsic, simdBaseJitType, genTypeSize(simdType));
929929
}
930930

‎src/coreclr/jit/simd.h‎

Lines changed: 51 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -453,7 +453,23 @@ void EvaluateUnarySimd(genTreeOps oper, bool scalar, var_types baseType, TSimd*
453453
template <typename TBase>
454454
TBase EvaluateBinaryScalarRSZ(TBase arg0, TBase arg1)
455455
{
456-
return arg0 >> (arg1 & ((sizeof(TBase) * 8) - 1));
456+
#if defined(TARGET_XARCH)
457+
if ((arg1 < 0) || (arg1 >= (sizeof(TBase) * 8)))
458+
{
459+
// For SIMD, xarch allows overshifting and treats
460+
// it as zeroing. So ensure we do the same here.
461+
//
462+
// The xplat APIs ensure the shiftAmount is masked
463+
// to be within range, so we can't hit this for them.
464+
465+
return static_cast<TBase>(0);
466+
}
467+
#else
468+
// Other platforms enforce masking in their encoding
469+
assert((arg1 >= 0) && (arg1 < (sizeof(TBase) * 8)));
470+
#endif
471+
472+
return arg0 >> arg1;
457473
}
458474

459475
template <>
@@ -513,7 +529,22 @@ TBase EvaluateBinaryScalarSpecialized(genTreeOps oper, TBase arg0, TBase arg1)
513529

514530
case GT_LSH:
515531
{
516-
return arg0 << (arg1 & ((sizeof(TBase) * 8) - 1));
532+
#if defined(TARGET_XARCH)
533+
if ((arg1 < 0) || (arg1 >= (sizeof(TBase) * 8)))
534+
{
535+
// For SIMD, xarch allows overshifting and treats
536+
// it as zeroing. So ensure we do the same here.
537+
//
538+
// The xplat APIs ensure the shiftAmount is masked
539+
// to be within range, so we can't hit this for them.
540+
541+
return static_cast<TBase>(0);
542+
}
543+
#else
544+
// Other platforms enforce masking in their encoding
545+
assert((arg1 >= 0) && (arg1 < (sizeof(TBase) * 8)));
546+
#endif
547+
return arg0 << arg1;
517548
}
518549

519550
case GT_OR:
@@ -535,7 +566,24 @@ TBase EvaluateBinaryScalarSpecialized(genTreeOps oper, TBase arg0, TBase arg1)
535566

536567
case GT_RSH:
537568
{
538-
return arg0 >> (arg1 & ((sizeof(TBase) * 8) - 1));
569+
#if defined(TARGET_XARCH)
570+
if ((arg1 < 0) || (arg1 >= (sizeof(TBase) * 8)))
571+
{
572+
// For SIMD, xarch allows overshifting and treats
573+
// it as propagating the sign bit (returning Zero
574+
// or AllBitsSet). So ensure we do the same here.
575+
//
576+
// The xplat APIs ensure the shiftAmount is masked
577+
// to be within range, so we can't hit this for them.
578+
579+
arg0 >>= ((sizeof(TBase) * 8) - 1);
580+
arg1 = static_cast<TBase>(1);
581+
}
582+
#else
583+
// Other platforms enforce masking in their encoding
584+
assert((arg1 >= 0) && (arg1 < (sizeof(TBase) * 8)));
585+
#endif
586+
return arg0 >> arg1;
539587
}
540588

541589
case GT_RSZ:

‎src/coreclr/jit/valuenum.cpp‎

Lines changed: 75 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -7518,6 +7518,31 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunBinary(var_types type,
75187518
case NI_AVX512BW_ShiftLeftLogical:
75197519
#endif
75207520
{
7521+
#ifdef TARGET_XARCH
7522+
if (TypeOfVN(arg1VN) == TYP_SIMD16)
7523+
{
7524+
// The xarch shift instructions support taking the shift amount as
7525+
// a simd16, in which case they take the shift amount from the lower
7526+
// 64-bits.
7527+
7528+
uint64_t shiftAmount = GetConstantSimd16(arg1VN).u64[0];
7529+
7530+
if (genTypeSize(baseType) != 8)
7531+
{
7532+
if (shiftAmount > INT_MAX)
7533+
{
7534+
// Ensure we don't lose track the the amount is an overshift
7535+
shiftAmount = -1;
7536+
}
7537+
arg1VN = VNForIntCon(static_cast<int32_t>(shiftAmount));
7538+
}
7539+
else
7540+
{
7541+
arg1VN = VNForLongCon(static_cast<int64_t>(shiftAmount));
7542+
}
7543+
}
7544+
#endif // TARGET_XARCH
7545+
75217546
return EvaluateBinarySimd(this, GT_LSH, /* scalar */ false, type, baseType, arg0VN, arg1VN);
75227547
}
75237548

@@ -7531,6 +7556,31 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunBinary(var_types type,
75317556
case NI_AVX512BW_ShiftRightArithmetic:
75327557
#endif
75337558
{
7559+
#ifdef TARGET_XARCH
7560+
if (TypeOfVN(arg1VN) == TYP_SIMD16)
7561+
{
7562+
// The xarch shift instructions support taking the shift amount as
7563+
// a simd16, in which case they take the shift amount from the lower
7564+
// 64-bits.
7565+
7566+
uint64_t shiftAmount = GetConstantSimd16(arg1VN).u64[0];
7567+
7568+
if (genTypeSize(baseType) != 8)
7569+
{
7570+
if (shiftAmount > INT_MAX)
7571+
{
7572+
// Ensure we don't lose track the the amount is an overshift
7573+
shiftAmount = -1;
7574+
}
7575+
arg1VN = VNForIntCon(static_cast<int32_t>(shiftAmount));
7576+
}
7577+
else
7578+
{
7579+
arg1VN = VNForLongCon(static_cast<int64_t>(shiftAmount));
7580+
}
7581+
}
7582+
#endif // TARGET_XARCH
7583+
75347584
return EvaluateBinarySimd(this, GT_RSH, /* scalar */ false, type, baseType, arg0VN, arg1VN);
75357585
}
75367586

@@ -7543,6 +7593,31 @@ ValueNum ValueNumStore::EvalHWIntrinsicFunBinary(var_types type,
75437593
case NI_AVX512BW_ShiftRightLogical:
75447594
#endif
75457595
{
7596+
#ifdef TARGET_XARCH
7597+
if (TypeOfVN(arg1VN) == TYP_SIMD16)
7598+
{
7599+
// The xarch shift instructions support taking the shift amount as
7600+
// a simd16, in which case they take the shift amount from the lower
7601+
// 64-bits.
7602+
7603+
uint64_t shiftAmount = GetConstantSimd16(arg1VN).u64[0];
7604+
7605+
if (genTypeSize(baseType) != 8)
7606+
{
7607+
if (shiftAmount > INT_MAX)
7608+
{
7609+
// Ensure we don't lose track the the amount is an overshift
7610+
shiftAmount = -1;
7611+
}
7612+
arg1VN = VNForIntCon(static_cast<int32_t>(shiftAmount));
7613+
}
7614+
else
7615+
{
7616+
arg1VN = VNForLongCon(static_cast<int64_t>(shiftAmount));
7617+
}
7618+
}
7619+
#endif // TARGET_XARCH
7620+
75467621
return EvaluateBinarySimd(this, GT_RSZ, /* scalar */ false, type, baseType, arg0VN, arg1VN);
75477622
}
75487623

‎src/tests/JIT/Regression/JitBlue/Runtime_93698/Runtime_93698.cs‎

Lines changed: 49 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,49 @@
1+
// Licensed to the .NET Foundation under one or more agreements.
2+
// The .NET Foundation licenses this file to you under the MIT license.
3+
4+
using System;
5+
using System.Runtime.Intrinsics;
6+
using System.Runtime.Intrinsics.X86;
7+
using Xunit;
8+
9+
public static class Runtime_93698
10+
{
11+
[Fact]
12+
public static void TestShiftLeftLogicalOvershift()
13+
{
14+
if (Sse2.IsSupported)
15+
{
16+
var result1 = Sse2.ShiftLeftLogical(Vector128.Create(-1, +2, -3, +4), 32);
17+
Assert.Equal(Vector128<int>.Zero, result1);
18+
19+
var result2 = Sse2.ShiftLeftLogical(Vector128.Create(-5, +6, -7, +8), Vector128.Create(0, 32, 0, 0));
20+
Assert.Equal(Vector128<int>.Zero, result2);
21+
}
22+
}
23+
24+
[Fact]
25+
public static void TestShiftRightLogicalOvershift()
26+
{
27+
if (Sse2.IsSupported)
28+
{
29+
var result1 = Sse2.ShiftRightLogical(Vector128.Create(-1, +2, -3, +4), 32);
30+
Assert.Equal(Vector128<int>.Zero, result1);
31+
32+
var result2 = Sse2.ShiftRightLogical(Vector128.Create(-5, +6, -7, +8), Vector128.Create(0, 32, 0, 0));
33+
Assert.Equal(Vector128<int>.Zero, result2);
34+
}
35+
}
36+
37+
[Fact]
38+
public static void TestShiftRightArithmeticOvershift()
39+
{
40+
if (Sse2.IsSupported)
41+
{
42+
var result1 = Sse2.ShiftRightArithmetic(Vector128.Create(-1, +2, -3, +4), 32);
43+
Assert.Equal(Vector128.Create(-1, 0, -1, 0), result1);
44+
45+
var result2 = Sse2.ShiftRightArithmetic(Vector128.Create(-5, +6, -7, +8), Vector128.Create(0, 32, 0, 0));
46+
Assert.Equal(Vector128.Create(-1, 0, -1, 0), result2);
47+
}
48+
}
49+
}

‎src/tests/JIT/Regression/JitBlue/Runtime_93698/Runtime_93698.csproj‎

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,8 @@
1+
<Project Sdk="Microsoft.NET.Sdk">
2+
<PropertyGroup>
3+
<Optimize>True</Optimize>
4+
</PropertyGroup>
5+
<ItemGroup>
6+
<Compile Include="$(MSBuildProjectName).cs" />
7+
</ItemGroup>
8+
</Project>

0 commit comments

Comments
 (0)