shader-slang / slang

Making it easier to work with shaders
http://shader-slang.com
Other
2.97k stars 210 forks source link

fmax3 implemented incorrectly for non-metal targets #5581

Open cheneym2 opened 1 week ago

cheneym2 commented 1 week ago

By inspection, this appears to have a bug where X is NaN and Y and Z are not specials.

fmin3() has a similar issue as described below for fmax3()

fmax3(NaN, 1, 2) should return 2 but would return 1.

__generic<T : __BuiltinFloatingPointType>
[__readNone]
[require(cpp_cuda_glsl_hlsl_metal_spirv, sm_4_0_version)]
T fmax3(T x, T y, T z)
{
    __target_switch
    {
    case metal: __intrinsic_asm "fmax3";
    default:
    {
        bool isnanX = isnan(x);
        bool isnanY = isnan(y);
        bool isnanZ = isnan(z);

        if (isnanX)
        {
            return isnanY ? z : y;
        }
        else if (isnanY)
        {
            if (isnanZ)
                return x;
            return max(x, z);
        }
        else if (isnanZ)
        {
            return max(x, y);
        }

        return max(y, max(x, z));
    }
    }
}
cheneym2 commented 1 week ago

See also https://github.com/shader-slang/slang/issues/5580 which would simplify the implementation of fmax3()

cheneym2 commented 1 week ago

I think fmax3() could be implemented purely as fmax(x, fmax(y, z)) if fmax() was fixed, per issue 5580

jkwak-work commented 6 days ago

As I said on the other issue, I think we should remove fmax3/fmin3 from hlsl.meta.slang