Skip to content

Fix side effect handling for certain TernaryLogic cases #114848

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
merged 1 commit into from
Apr 23, 2025
Merged

Fix side effect handling for certain TernaryLogic cases #114848

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
65 changes: 61 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 @@ -4405,6 +4405,65 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
}
}

// Some normalization cases require us to swap the operands, which might require
// spilling side effects. Check that here.
//
// cast in switch clause is needed for old gcc
switch ((TernaryLogicOperKind)info.oper1)
{
case TernaryLogicOperKind::Not:
{
assert(info.oper1Use != TernaryLogicUseFlags::None);

bool needSideEffectSpill = false;

if (info.oper2 == TernaryLogicOperKind::And)
{
assert(info.oper2Use != TernaryLogicUseFlags::None);

if ((control == static_cast<uint8_t>(~0xCC & 0xF0)) || // ~B & A
(control == static_cast<uint8_t>(~0xAA & 0xF0)) || // ~C & A
(control == static_cast<uint8_t>(~0xAA & 0xCC))) // ~C & B
{
// We're normalizing to ~B & C, so we need another swap
std::swap(val2, val3);
needSideEffectSpill = (control == static_cast<uint8_t>(~0xAA & 0xCC)); // ~C & B
}
}
else if (info.oper2 == TernaryLogicOperKind::Or)
{
assert(info.oper2Use != TernaryLogicUseFlags::None);

if ((control == static_cast<uint8_t>(~0xCC | 0xF0)) || // ~B | A
(control == static_cast<uint8_t>(~0xAA | 0xF0)) || // ~C | A
(control == static_cast<uint8_t>(~0xAA | 0xCC))) // ~C | B
{
// We're normalizing to ~B | C, so we need another swap
std::swap(val2, val3);
needSideEffectSpill = (control == static_cast<uint8_t>(~0xAA | 0xCC)); // ~C | B
}
}

if (needSideEffectSpill)
{
// Side-effect cases:
// ~B op A ; order before swap C A B
// op1 & op2 already set to be spilled; no further spilling necessary
// ~C op A ; order before swap B A C
// op1 already set to be spilled; no further spilling necessary
// ~C op B ; order before swap A B C
// nothing already set to be spilled; op1 & op2 need to be spilled

spillOp1 = true;
spillOp2 = true;
}
break;
}

default:
break;
}

if (spillOp1)
{
impSpillSideEffect(true, stackState.esStackDepth -
Expand Down Expand Up @@ -4529,8 +4588,7 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
(control == static_cast<uint8_t>(~0xAA & 0xF0)) || // ~C & A
(control == static_cast<uint8_t>(~0xAA & 0xCC))) // ~C & B
{
// We're normalizing to ~B & C, so we need another swap
std::swap(*val2, *val3);
// We already normalized to ~B & C above.
Copy link
Member

@tannergooding tannergooding Apr 23, 2025

Choose a reason for hiding this comment

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

nit: This first if could be merged with the assert below. We may not even need the assert at all since it's the only possible 6 permutations of NOT AND and NOT OR

Copy link
Contributor Author

@BruceForstall BruceForstall Apr 23, 2025

Choose a reason for hiding this comment

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

I kept the logic here stylized to be the same as the hoisted logic, for better or worse...

}
else
{
Expand All @@ -4556,8 +4614,7 @@ GenTree* Compiler::impSpecialIntrinsic(NamedIntrinsic intrinsic,
(control == static_cast<uint8_t>(~0xAA | 0xF0)) || // ~C | A
(control == static_cast<uint8_t>(~0xAA | 0xCC))) // ~C | B
{
// We're normalizing to ~B & C, so we need another swap
std::swap(*val2, *val3);
// We already normalized to ~B | C above.
}
else
{
Expand Down
51 changes: 51 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_114324/Runtime_114324.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,51 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Generated by Fuzzlyn v2.5 on 2025-04-06 16:30:45
// Run on X64 Windows
// Seed: 13621799076737484256-vectort,vector128,vector256,x86aes,x86avx,x86avx2,x86avx512bw,x86avx512bwvl,x86avx512cd,x86avx512cdvl,x86avx512dq,x86avx512dqvl,x86avx512f,x86avx512fvl,x86avx512fx64,x86bmi1,x86bmi1x64,x86bmi2,x86bmi2x64,x86fma,x86lzcnt,x86lzcntx64,x86pclmulqdq,x86popcnt,x86popcntx64,x86sse,x86ssex64,x86sse2,x86sse2x64,x86sse3,x86sse41,x86sse41x64,x86sse42,x86sse42x64,x86ssse3,x86x86base
// Reduced from 20.1 KiB to 0.7 KiB in 00:01:13
// Debug: Runs successfully
// Release: Throws 'System.NullReferenceException'
using System;
using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.X86;
using Xunit;

public class Runtime_114324
{
public static ulong[] s_3;

[Fact]
public static void TestEntryPoint()
{
if (Avx512F.VL.IsSupported)
{
var vr1 = Vector128.Create<int>(1);
Vector128<int> vr2 = Avx512F.VL.TernaryLogic(
vr1,
Sse2.ShiftLeftLogical128BitLane(
Vector128.Create<int>((int)Sse42.Crc32(M2(), 0)),
0),
Avx512F.VL.ConvertToVector128Int32(Vector256.Create<ulong>(s_3[0])),
221);
System.Console.WriteLine(vr2);
Assert.Equal(Vector128.Create(-1,-1,-1,-1), vr2);
}
}

public static uint M2()
{
s_3 = new ulong[]
{
0
};
return M3();
}

public static byte M3()
{
return default(byte);
}
}
8 changes: 8 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_114324/Runtime_114324.csproj
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
<Project Sdk="Microsoft.NET.Sdk">
<PropertyGroup>
<Optimize>True</Optimize>
</PropertyGroup>
<ItemGroup>
<Compile Include="$(MSBuildProjectName).cs" />
</ItemGroup>
</Project>
Loading