Skip to content
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.

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

JIT: Fix over-shifting logic when constant-folding AdvSimd.ShiftRight* and AdvSimd.ShiftLeft* #105847

Merged
merged 8 commits into from
Aug 5, 2024
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
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
8 changes: 4 additions & 4 deletions src/coreclr/jit/simd.h
Original file line number Diff line number Diff line change
Expand Up @@ -668,10 +668,10 @@ void EvaluateUnarySimd(genTreeOps oper, bool scalar, var_types baseType, TSimd*
template <typename TBase>
TBase EvaluateBinaryScalarRSZ(TBase arg0, TBase arg1)
{
#if defined(TARGET_XARCH)
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
if ((arg1 < 0) || (arg1 >= (sizeof(TBase) * 8)))
{
// For SIMD, xarch allows overshifting and treats
// For SIMD, xarch and ARM64 allow overshifting and treat
// it as zeroing. So ensure we do the same here.
//
// The xplat APIs ensure the shiftAmount is masked
Expand Down Expand Up @@ -817,10 +817,10 @@ TBase EvaluateBinaryScalarSpecialized(genTreeOps oper, TBase arg0, TBase arg1)

case GT_RSH:
{
#if defined(TARGET_XARCH)
#if defined(TARGET_XARCH) || defined(TARGET_ARM64)
if ((arg1 < 0) || (arg1 >= (sizeof(TBase) * 8)))
{
// For SIMD, xarch allows overshifting and treats
// For SIMD, xarch and ARM64 allow overshifting and treat
// it as propagating the sign bit (returning Zero
// or AllBitsSet). So ensure we do the same here.
//
Expand Down
60 changes: 60 additions & 0 deletions src/tests/JIT/Regression/JitBlue/Runtime_105817/Runtime_105817.cs
Original file line number Diff line number Diff line change
@@ -0,0 +1,60 @@
// 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.2 on 2024-08-01 14:37:47
// Run on Arm64 MacOS
// Seed: 14773448547728333023-vectort,vector64,vector128,armadvsimd,armadvsimdarm64,armaes,armarmbase,armarmbasearm64,armcrc32,armcrc32arm64,armdp,armrdm,armrdmarm64,armsha1,armsha256
// Reduced from 270.2 KiB to 0.4 KiB in 00:01:48
// Debug: Outputs 0
// Release: Outputs 1
using System;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;
using Xunit;

public class Runtime_105817
{
[Fact]
public static void TestOverShiftRightLogical()
{
if (AdvSimd.IsSupported)
{
var vr6 = Vector128.Create<short>(1);
var vr7 = AdvSimd.ShiftRightLogical(vr6, 16);
Assert.Equal(vr7, Vector128<short>.Zero);
}
}

[Fact]
public static void TestOverShiftRightLogicalScalar()
{
if (AdvSimd.IsSupported)
{
var vr6 = Vector64.Create<long>(1);
var vr7 = AdvSimd.ShiftRightLogicalScalar(vr6, 64);
Copy link
Member

@tannergooding tannergooding Aug 1, 2024

Choose a reason for hiding this comment

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

Can we also check an out of range value like 65? With the other change it should now be executing the ShiftLogical path rather than throwing, so we want to ensure its computing the right result between the unoptimized vs optimized as well.

It may be interesting to check it for a non-constant or a value that only becomes constant after importation as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure thing. #105777 only added the fallback implementation for ShiftRightLogical, so ShiftRightLogicalScalar, ShiftRightArithmetic, and ShiftRightArithmeticScalar still throw in Debug. Are you ok with me adding their fallback implementations in this PR, or would you prefer that work in a separate one?

Copy link
Member Author

Choose a reason for hiding this comment

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

We weren't using the fallback APIs for non-const immediates either, so I added the HW_Flag_NoJmpTableIMM flag to the ShiftRight* APIs so we always use the fallback instead of a jump table.

Assert.Equal(vr7, Vector64<long>.Zero);
}
}

[Fact]
public static void TestOverShiftRightArithmetic()
{
if (AdvSimd.IsSupported)
{
var vr6 = Vector128.Create<short>(1);
var vr7 = AdvSimd.ShiftRightArithmetic(vr6, 16);
Assert.Equal(vr7, Vector128<short>.Zero);
}
}

[Fact]
public static void TestOverShiftRightArithmeticScalar()
{
if (AdvSimd.IsSupported)
{
var vr6 = Vector64.Create<long>(1);
var vr7 = AdvSimd.ShiftRightArithmeticScalar(vr6, 64);
Assert.Equal(vr7, Vector64<long>.Zero);
}
}
}
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