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

fmax3 implemented incorrectly for non-metal targets #5581

Open
cheneym2 opened this issue Nov 18, 2024 · 3 comments
Open

fmax3 implemented incorrectly for non-metal targets #5581

cheneym2 opened this issue Nov 18, 2024 · 3 comments
Assignees
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang GoodFirstBug Great bug for people getting going in slang codebase kind:bug something doesn't work like it should

Comments

@cheneym2
Copy link
Collaborator

cheneym2 commented Nov 18, 2024

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 cheneym2 added this to the Q4 2024 (Fall) milestone Nov 18, 2024
@cheneym2
Copy link
Collaborator Author

See also #5580 which would simplify the implementation of fmax3()

@cheneym2 cheneym2 added GoodFirstBug Great bug for people getting going in slang codebase kind:bug something doesn't work like it should labels Nov 18, 2024
@cheneym2
Copy link
Collaborator Author

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

@jkwak-work
Copy link
Collaborator

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

@bmillsNV bmillsNV added the goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang label Nov 21, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
goal:quality & productivity Quality issues and issues that impact our productivity coding day to day inside slang GoodFirstBug Great bug for people getting going in slang codebase kind:bug something doesn't work like it should
Projects
None yet
Development

No branches or pull requests

3 participants