JIT: Bad result with AdvSimd.ShiftRightLogical

[jakobbotsch]

// 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.CompilerServices;
using System.Numerics;
using System.Runtime.Intrinsics;
using System.Runtime.Intrinsics.Arm;

public class Program
{
    public static void Main()
    {
        var vr6 = Vector128.Create<short>(1);
        var vr7 = AdvSimd.ShiftRightLogical(vr6, 16);
        var vr8 = AdvSimd.Extract(vr7, 0);
        System.Console.WriteLine(vr8);
    }
}

cc @dotnet/jit-contrib

[dotnet-policy-service]

Tagging subscribers to this area: @JulieLeeMSFT, @jakobbotsch
See info in area-owners.md if you want to be subscribed.

[amanasifkhalid]

I can take this. I suspect this is related to my fix for #105621.

[amanasifkhalid]

Debug codegen is correct. We just emit the intrinsic as so: ushr v16.8h, v16.8h, #16. In Release, we constant-fold the intrinsic, and the evaluation is handled by EvaluateBinaryScalarRSZ. This method has a comment explaining overshifting is allowed for SIMD only on xarch, while all other platforms mask the immediate to avoid overshifting. Thus, we constant-fold the intrinsic with a shift amount of 16 & 15, or 0, hence the wrong output.

@tannergooding ShiftRightLogical and ShiftRightLogicalScalar are the only AdvSimd intrinsics for which GenTreeHWIntrinsic::GetOperForHWIntrinsicId returns GT_RSZ, and they both have immediate bounds of [1, sizeof(T) * 8], so it should be safe to extend the overshifting behavior in EvaluateBinaryScalarRSZ to ARM64 intrinsics, right?

[tannergooding]

If the intrinsics are overshifting here, then yes and it's likely that the comment is just wrong (because RSZ for SIMD is [1, sizeof(T) * 8] not [0, sizeof(T) * 8 - 1] like it is for LSH/RSH).

It might be good to double check the behavior of ShiftLogical here as well, I believe it takes the first 8-bits, but we shouldn't be constant folding those today and intrinsics like AdvSimd.ShiftRightLogical should be taking the shift amount as byte (so restricted to 8-bits already).

[tannergooding]

May also need to double check ShiftRightArithmetic, looks like SSHR is also [1, sizeof(T) * 8]

[amanasifkhalid]

May also need to double check ShiftRightArithmetic, looks like SSHR is also [1, sizeof(T) * 8]

You're right, the GT_RSH path also had to be updated. I've added test coverage for both paths in #105847.

It might be good to double check the behavior of ShiftLogical here as well, I believe it takes the first 8-bits, but we shouldn't be constant folding those today and intrinsics like AdvSimd.ShiftRightLogical should be taking the shift amount as byte (so restricted to 8-bits already).

I believe you're correct. The docs say ShiftLogical reads only the LSB of the register containing the immediate, and there doesn't seem to be any constant folding path for it (GenTreeHWIntrinsic::GetOperForHWIntrinsicId returns GT_NONE for it), so I don't think anything needs to be done for that intrinsic.