Skip to content

Commit 4b035c9

Browse files
tannergoodingtannergooding
authored
[release/6.0] Ensure Max/Min for floating-point on x86/x64 are not handled as commutative (#75772)
* Ensure Max/Min for floating-point on x86/x64 are not handled as commutative (#75683) * Adding a regression test for SixLabors/ImageSharp#2117 * Ensure Max/Min for floating-point on x86/x64 are not handled as commutative * Applying formatting patch * Directly access the gtHWIntrinsicId field since the helper method doesn't exist in .NET 6 * Fixing a regression test to use the GetElement API available in .NET 6
1 parent 3c55f1d commit 4b035c9

File tree

5 files changed

+111
-9
lines changed

5 files changed

+111
-9
lines changed

‎src/coreclr/jit/gentree.cpp

Lines changed: 38 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -18912,10 +18912,45 @@ bool GenTree::isCommutativeHWIntrinsic() const
1891218912
assert(gtOper == GT_HWINTRINSIC);
1891318913

1891418914
#ifdef TARGET_XARCH
18915-
return HWIntrinsicInfo::IsCommutative(AsHWIntrinsic()->gtHWIntrinsicId);
18916-
#else
18917-
return false;
18915+
const GenTreeHWIntrinsic* node = AsHWIntrinsic();
18916+
NamedIntrinsic id = node->gtHWIntrinsicId;
18917+
18918+
if (HWIntrinsicInfo::IsCommutative(id))
18919+
{
18920+
return true;
18921+
}
18922+
18923+
if (HWIntrinsicInfo::IsMaybeCommutative(id))
18924+
{
18925+
switch (id)
18926+
{
18927+
case NI_SSE_Max:
18928+
case NI_SSE_Min:
18929+
{
18930+
return false;
18931+
}
18932+
18933+
case NI_SSE2_Max:
18934+
case NI_SSE2_Min:
18935+
{
18936+
return !varTypeIsFloating(node->GetSimdBaseType());
18937+
}
18938+
18939+
case NI_AVX_Max:
18940+
case NI_AVX_Min:
18941+
{
18942+
return false;
18943+
}
18944+
18945+
default:
18946+
{
18947+
unreached();
18948+
}
18949+
}
18950+
}
1891818951
#endif // TARGET_XARCH
18952+
18953+
return false;
1891918954
}
1892018955

1892118956
bool GenTree::isContainableHWIntrinsic() const

‎src/coreclr/jit/hwintrinsic.h

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -141,6 +141,11 @@ enum HWIntrinsicFlag : unsigned int
141141
// all the intrinsic that have explicit memory load/store semantics should have this flag
142142
HW_Flag_NoContainment = 0x8000,
143143

144+
// MaybeCommutative
145+
// - if a binary-op intrinsic is maybe commutative (e.g., Max or Min for float/double), its op1 can possibly be
146+
// contained
147+
HW_Flag_MaybeCommutative = 0x80000,
148+
144149
#elif defined(TARGET_ARM64)
145150
// The intrinsic has an immediate operand
146151
// - the value can be (and should be) encoded in a corresponding instruction when the operand value is constant
@@ -597,6 +602,18 @@ struct HWIntrinsicInfo
597602
return (flags & HW_Flag_Commutative) != 0;
598603
}
599604

605+
static bool IsMaybeCommutative(NamedIntrinsic id)
606+
{
607+
HWIntrinsicFlag flags = lookupFlags(id);
608+
#if defined(TARGET_XARCH)
609+
return (flags & HW_Flag_MaybeCommutative) != 0;
610+
#elif defined(TARGET_ARM64)
611+
return false;
612+
#else
613+
#error Unsupported platform
614+
#endif
615+
}
616+
600617
static bool RequiresCodegen(NamedIntrinsic id)
601618
{
602619
HWIntrinsicFlag flags = lookupFlags(id);

‎src/coreclr/jit/hwintrinsiclistxarch.h

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -158,9 +158,9 @@ HARDWARE_INTRINSIC(SSE, LoadHigh,
158158
HARDWARE_INTRINSIC(SSE, LoadLow, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movlps, INS_invalid}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
159159
HARDWARE_INTRINSIC(SSE, LoadScalarVector128, 16, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movss, INS_invalid}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
160160
HARDWARE_INTRINSIC(SSE, LoadVector128, 16, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movups, INS_invalid}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
161-
HARDWARE_INTRINSIC(SSE, Max, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxps, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_Commutative)
161+
HARDWARE_INTRINSIC(SSE, Max, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxps, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_MaybeCommutative)
162162
HARDWARE_INTRINSIC(SSE, MaxScalar, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxss, INS_invalid}, HW_Category_SIMDScalar, HW_Flag_CopyUpperBits)
163-
HARDWARE_INTRINSIC(SSE, Min, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minps, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_Commutative)
163+
HARDWARE_INTRINSIC(SSE, Min, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minps, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_MaybeCommutative)
164164
HARDWARE_INTRINSIC(SSE, MinScalar, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minss, INS_invalid}, HW_Category_SIMDScalar, HW_Flag_CopyUpperBits)
165165
HARDWARE_INTRINSIC(SSE, MoveHighToLow, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movhlps, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_NoContainment)
166166
HARDWARE_INTRINSIC(SSE, MoveLowToHigh, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movlhps, INS_invalid}, HW_Category_SimpleSIMD, HW_Flag_NoContainment)
@@ -271,10 +271,10 @@ HARDWARE_INTRINSIC(SSE2, LoadLow,
271271
HARDWARE_INTRINSIC(SSE2, LoadScalarVector128, 16, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movd, INS_movd, INS_movq, INS_movq, INS_invalid, INS_movsdsse2}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
272272
HARDWARE_INTRINSIC(SSE2, LoadVector128, 16, 1, {INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_invalid, INS_movupd}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
273273
HARDWARE_INTRINSIC(SSE2, MaskMove, 16, 3, {INS_maskmovdqu, INS_maskmovdqu, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_MemoryStore, HW_Flag_NoContainment|HW_Flag_NoRMWSemantics|HW_Flag_BaseTypeFromSecondArg)
274-
HARDWARE_INTRINSIC(SSE2, Max, 16, 2, {INS_invalid, INS_pmaxub, INS_pmaxsw, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxpd}, HW_Category_SimpleSIMD, HW_Flag_Commutative)
274+
HARDWARE_INTRINSIC(SSE2, Max, 16, 2, {INS_invalid, INS_pmaxub, INS_pmaxsw, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxpd}, HW_Category_SimpleSIMD, HW_Flag_MaybeCommutative)
275275
HARDWARE_INTRINSIC(SSE2, MemoryFence, 0, 0, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid}, HW_Category_Special, HW_Flag_NoContainment|HW_Flag_NoRMWSemantics)
276276
HARDWARE_INTRINSIC(SSE2, MaxScalar, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxsd}, HW_Category_SIMDScalar, HW_Flag_CopyUpperBits)
277-
HARDWARE_INTRINSIC(SSE2, Min, 16, 2, {INS_invalid, INS_pminub, INS_pminsw, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minpd}, HW_Category_SimpleSIMD, HW_Flag_Commutative)
277+
HARDWARE_INTRINSIC(SSE2, Min, 16, 2, {INS_invalid, INS_pminub, INS_pminsw, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minpd}, HW_Category_SimpleSIMD, HW_Flag_MaybeCommutative)
278278
HARDWARE_INTRINSIC(SSE2, MinScalar, 16, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minsd}, HW_Category_SIMDScalar, HW_Flag_CopyUpperBits)
279279
HARDWARE_INTRINSIC(SSE2, MoveMask, 16, 1, {INS_pmovmskb, INS_pmovmskb, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movmskpd}, HW_Category_SimpleSIMD, HW_Flag_NoContainment|HW_Flag_NoRMWSemantics|HW_Flag_BaseTypeFromFirstArg)
280280
HARDWARE_INTRINSIC(SSE2, MoveScalar, 16, -1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movq, INS_movq, INS_invalid, INS_movsdsse2}, HW_Category_SIMDScalar, HW_Flag_NoContainment)
@@ -466,8 +466,8 @@ HARDWARE_INTRINSIC(AVX, InsertVector128,
466466
HARDWARE_INTRINSIC(AVX, LoadAlignedVector256, 32, 1, {INS_movdqa, INS_movdqa, INS_movdqa, INS_movdqa, INS_movdqa, INS_movdqa, INS_movdqa, INS_movdqa, INS_movaps, INS_movapd}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
467467
HARDWARE_INTRINSIC(AVX, LoadDquVector256, 32, 1, {INS_lddqu, INS_lddqu, INS_lddqu, INS_lddqu, INS_lddqu, INS_lddqu, INS_lddqu, INS_lddqu, INS_invalid, INS_invalid}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
468468
HARDWARE_INTRINSIC(AVX, LoadVector256, 32, 1, {INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movdqu, INS_movups, INS_movupd}, HW_Category_MemoryLoad, HW_Flag_NoRMWSemantics)
469-
HARDWARE_INTRINSIC(AVX, Max, 32, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxps, INS_maxpd}, HW_Category_SimpleSIMD, HW_Flag_Commutative)
470-
HARDWARE_INTRINSIC(AVX, Min, 32, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minps, INS_minpd}, HW_Category_SimpleSIMD, HW_Flag_Commutative)
469+
HARDWARE_INTRINSIC(AVX, Max, 32, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_maxps, INS_maxpd}, HW_Category_SimpleSIMD, HW_Flag_MaybeCommutative)
470+
HARDWARE_INTRINSIC(AVX, Min, 32, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_minps, INS_minpd}, HW_Category_SimpleSIMD, HW_Flag_MaybeCommutative)
471471
HARDWARE_INTRINSIC(AVX, MaskLoad, -1, 2, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vmaskmovps, INS_vmaskmovpd}, HW_Category_MemoryLoad, HW_Flag_NoFlag)
472472
HARDWARE_INTRINSIC(AVX, MaskStore, -1, 3, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_vmaskmovps, INS_vmaskmovpd}, HW_Category_MemoryStore, HW_Flag_NoContainment|HW_Flag_BaseTypeFromSecondArg)
473473
HARDWARE_INTRINSIC(AVX, MoveMask, 32, 1, {INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_invalid, INS_movmskps, INS_movmskpd}, HW_Category_SimpleSIMD, HW_Flag_NoContainment|HW_Flag_BaseTypeFromFirstArg)

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

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
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.Runtime.Intrinsics;
5+
using System.Runtime.Intrinsics.X86;
6+
7+
public unsafe class ImageSharp_2117
8+
{
9+
public static int Main(string[] args)
10+
{
11+
if (Sse.IsSupported)
12+
{
13+
Vector128<float> fnan = Vector128.Create(float.NaN);
14+
Vector128<float> res1 = Sse.Max(Sse.LoadVector128((float*)(&fnan)), Vector128<float>.Zero);
15+
Vector128<float> res2 = Sse.Min(Sse.LoadVector128((float*)(&fnan)), Vector128<float>.Zero);
16+
17+
if (float.IsNaN(res1.GetElement(0)) || float.IsNaN(res2.GetElement(0)))
18+
{
19+
return 0;
20+
}
21+
}
22+
23+
if (Sse2.IsSupported)
24+
{
25+
Vector128<double> dnan = Vector128.Create(double.NaN);
26+
Vector128<double> res3 = Sse2.Max(Sse2.LoadVector128((double*)(&dnan)), Vector128<double>.Zero);
27+
Vector128<double> res4 = Sse2.Min(Sse2.LoadVector128((double*)(&dnan)), Vector128<double>.Zero);
28+
29+
if (double.IsNaN(res3.GetElement(0)) || double.IsNaN(res4.GetElement(0)))
30+
{
31+
return 0;
32+
}
33+
}
34+
35+
return 100;
36+
}
37+
}

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

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

0 commit comments

Comments
 (0)