JIT: AdvSimd.ShiftRightLogical(x, 0) throws in debug but not in release

[jakobbotsch]

// Generated by Fuzzlyn v2.1 on 2024-07-28 20:49:49
// Run on Arm64 Linux
// Seed: 14204794442367797079-vectort,vector64,vector128,armadvsimd,armadvsimdarm64,armaes,armarmbase,armarmbasearm64,armcrc32,armcrc32arm64,armdp,armrdm,armrdmarm64,armsha1,armsha256
// Reduced from 12.6 KiB to 0.4 KiB in 00:00:17
// Debug: Throws 'System.ArgumentOutOfRangeException'
// Release: Runs successfully
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 vr3 = Vector64.Create<byte>(0);
        var vr4 = AdvSimd.ShiftRightLogical(vr3, 0);
        System.Console.WriteLine(vr4);
    }
}

cc @dotnet/jit-contrib

This is with #105527 included. Seems like it gets constant folded even though it should throw.

[dotnet-policy-service]

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

[JulieLeeMSFT]

@amanasifkhalid, PTAL.

[amanasifkhalid]

The reason why #105527 didn't cover AdvSimd.ShiftRightLogical seems to have to do with the fact that GenTreeHWIntrinsic::GetOperForHWIntrinsicId returns something other than GT_NONE for it. We have multiple paths (such as in Compiler::gtFoldExprHWIntrinsic, and ValueNumStore::EvalHWIntrinsicFunBinary, and maybe elsewhere) where we check if the intrinsic's operand is something other than GT_NONE, which then unlocks folding later on -- even if the immediate is out of range. Regardless of what the intrinsic's operand is, I'm guessing we shouldn't be going down these paths at all if the intrinsic might throw (i.e. if it's marked with GTF_HW_USER_CALL).

[tannergooding]

I don't think we want to just filter any GTF_HW_USER_CALL, as there are many cases where this may get marked and we get an in range constant. We're actually supposed to be checking for known out of range values and filtering those ourselves, you can see some examples of that around GetElement as an example: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/valuenum.cpp#L8280 and https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/gentree.cpp#L30966

The actual bug here is that debug is throwing, because it shouldn't be. AdvSimd.ShiftRightLogical maps to USHR (immediate), which due to encoding size limitations (https://developer.arm.com/documentation/ddi0602/2024-06/SIMD-FP-Instructions/USHR--Unsigned-shift-right--immediate--?lang=en) can only represent constants in the range [1, sizeof(T) * 8]

When the value is not a constant or not in range, it should instead be switching over to using AdvSimd.ShiftLogical which maps USHL (register): https://developer.arm.com/documentation/ddi0602/2024-06/SIMD-FP-Instructions/USHL--Unsigned-shift-left--register--?lang=en and which does support 0 -- right shifts are done via negative inputs, so we transform by doing something like gtNewSimdCreateBroadcastNode(simdType, gtNewOperNode(GT_NEG, op2), simdBaseJitType, genTypeSize(simdType))

This should be getting handled in impNonConstFallback (https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsicarm64.cpp#L565-L568), but it isn't. -- You can see several similar scenarios being handled in xarch here: https://github.com/dotnet/runtime/blob/main/src/coreclr/jit/hwintrinsicxarch.cpp. This is done because its purely an encoding limitation and the behavior is still possible and well defined for the architecture otherwise.

So, x >> 0 should just return x in debug, as it already does for release mode.

[amanasifkhalid]

@tannergooding thank you for the clarification! I'm guessing we should support this fallback for ShiftLeftLogical, too. Do we want to add fallback implementations for the other shift variants while we're at it (ShiftLogicalSaturate, ShiftLogicalScalar, etc)?

[tannergooding]

Do we want to add fallback implementations for the other shift variants while we're at it (ShiftLogicalSaturate, ShiftLogicalScalar, etc)?

We want them to exist where there is a trivial alternative instruction that doesn't have the requirement the input be a constant. For .NET 9, I think it's sufficient to do the minimal work here to ensure we have deterministic behavior between the debug/release, that way we aren't regressing any scenarios.

I think that others like ShiftLogicalSaturate which don't have any constant folding support can wait for .NET 10, unless they are particularly trivial to also add support for.

[amanasifkhalid]

Got it. I'm not seeing any discrepancy between debug/release for ShiftLeftLogical, so I'll stick to ShiftRightLogical for now.

[tannergooding]

Got it. I'm not seeing any discrepancy between debug/release for ShiftLeftLogical, so I'll stick to ShiftRightLogical for now.

👍, this is probably because SHL (immediate) is in the range [0, sizeof(T) * 8 - 1] and so includes 0 as valid. It's one of the weird "quirks" of the Arm64 ISA that SHL allows 0 but USHR does not

[amanasifkhalid]

this is probably because SHL (immediate) is in the range [0, sizeof(T) * 8 - 1] and so includes 0 as valid. It's one of the weird "quirks" of the Arm64 ISA that SHL allows 0 but USHR does not

Sorry just to clarify, I tried ShiftLeftLogical with out-of-bounds immediates (ex: 8 when operating on bytes), and it threw in both Debug and Release. So I'm guessing we aren't doing any constant folding for left shifts when optimizing.