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

math_brute_force: fix fdim to use device's rounding when converting result back to half. #2223

Merged
merged 1 commit into from
Jan 28, 2025

Conversation

cycheng
Copy link
Contributor

@cycheng cycheng commented Jan 10, 2025

In the half-precision fdim test, the original code used CL_HALF_RTE to convert the float result back to half, causing a mismatch in computation results when the hardware uses RTZ. Some of the examples:

  fdim(0x365f, 0xdc63) = fdim( 0.398193f,  -280.75f)     =   281.148193f (RTE=0x5c65, RTZ=0x5c64)
  fdim(0xa4a3, 0xf0e9) = fdim(-0.018112f, 10056.0f)      = 10055.981445f (RTE=0x70e9, RTZ=0x70e8)
  fdim(0x1904, 0x9ab7) = fdim( 0.002449f,    -0.003279f) =     0.005728f (RTE=0x1dde, RTZ=0x1ddd)

Fixed this by using the hardware's default rounding mode when converting the result back to half.

@cycheng
Copy link
Contributor Author

cycheng commented Jan 14, 2025

Thank you @svenvh, in the current change, we only set halfRoundingMode to CL_HALF_RTZ when both isFDim and gIsInRTZMode are true.

I can try to apply the change to all binary f16 operations and redo the CTS tests on my side.

@cycheng
Copy link
Contributor Author

cycheng commented Jan 14, 2025

@svenvh
CC @AhmedAmraniAkdi , @paulfradgley

Change halfRoundingMode to CL_HALF_RTZ for all binary operations, I was seeing 4 test failures on our RTZ device:

  • hypot

    • Case1:
      inputs = 0xfbe7, 0xfabb = -64736, -55136
      double r = std::hypot(-64736, -55136) = 85033.6875
      cl_half_from_float(r, CL_HALF_RTZ) = 0x7bff = 65504
      cl_half_from_float(r, CL_HALF_RTE) = 0x7c00 = inf
      ulp = -304.651367
    • Case2:
      inputs = 0xfbfe, 0x7387 = -65472, 15416
      double r = std::hypot(-65472, 15416) = 67262.4375
      cl_half_from_float(r, CL_HALF_RTZ) = 0x7bff = 65504
      cl_half_from_float(r, CL_HALF_RTE) = 0x7c00 = inf
      ulp = -26.975624
  • ldexp

  • Case 1:
    inputs = 0x77d5, 162
    fesetround(FE_TOWARDZERO)
    double r = std::ldexp(32080, 162) = 3.402823e+38
    cl_half_from_float(r, CL_HALF_RTZ) = 0x7bff = 65504
    cl_half_from_float(r, CL_HALF_RTE) = 0x7c00 = inf
    ulp = -2004.999877

  • pow

    • Case 1:
      inputs = 0x3750, 0xcfc2
      double r = std::pow(0.45703125, -31.03125) = 3.567007e+10
      cl_half_from_float(r, CL_HALF_RTZ) = 0x7bff = 65504
      cl_half_from_float(r, CL_HALF_RTE) = 0x7c00 = inf
      ulp = -1063.048706
  • powr

    • ..

Copy link
Contributor

@bashbaug bashbaug left a comment

Choose a reason for hiding this comment

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

LGTM

@bashbaug
Copy link
Contributor

This has three approvals from three different companies, so no reason to wait to merge it.

@bashbaug bashbaug merged commit 5749818 into KhronosGroup:main Jan 28, 2025
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants