Skip to content

[HIP] Fix double to E8M0 conversion underflow (avoid unsigned exp wraparound) - #2461

Merged
geomin12 merged 8 commits into
ROCm:developfrom
sshlyapn:double_to_e8m0__convert_fix
Feb 9, 2026
Merged

geomin12 merged 8 commits into
ROCm:developfrom
sshlyapn:double_to_e8m0__convert_fix

Conversation

@sshlyapn

@sshlyapn sshlyapn commented Dec 30, 2025 •

Copy link
Copy Markdown
Contributor

This PR fixes an exponent underflow bug in the FP8 E8M0 conversion path for doubles

Motivation

Currently, __hip_cvt_double_to_e8m0() computes the float-domain exponent using unsigned short arithmetic. For small normal double exponents (non-zero but less than the bias delta), this subtraction underflowed, producing a large value and incorrectly taking NaN path

Technical Details

  • Compute the exponent delta in signed arithmetic and clamp float_exp <= 0 to 0x0U (underflow to float subnormal/zero domain)
  • Preserve overflow/NaN/Inf handling via float_exp > 0xFF -> e8m0_NaN, and leave the existing rounding/saturation post-processing intact
  • Updated Catch2 matchers as Catch::Matchers::WithinAbs/WithinRel to match current Catch2 API

Test Plan

Test Result

  • Local: verified by running the FP8 E8M0 unit test locally; all checks passed
  • CI: not run (test is currently disabled)

Submission Checklist

Copilot AI review requested due to automatic review settings December 30, 2025 07:18
@sshlyapn
sshlyapn requested review from a team as code owners December 30, 2025 07:18
@sshlyapn

Copy link
Copy Markdown
Contributor Author

@AidanBeltonS I noticed you've worked on the FP8_E8M0 data type. Could you please review these changes?

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes an exponent underflow bug in the double-to-E8M0 conversion function where unsigned arithmetic caused wraparound for small normal double exponents, incorrectly classifying them as NaN.

Key Changes:

  • Replace unsigned exponent arithmetic with signed arithmetic to prevent underflow wraparound
  • Add explicit clamping logic for underflow (float_exp <= 0), valid range, and overflow cases
  • Extend test coverage with edge cases for subnormal and small normal doubles
  • Update Catch2 matcher syntax to current API (Catch::Matchers::WithinAbs/WithinRel)

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
projects/clr/hipamd/include/hip/amd_detail/amd_hip_fp8.h Refactored __hip_cvt_double_to_e8m0() to use signed arithmetic for exponent conversion, preventing unsigned underflow and correctly handling small normal doubles
projects/hip-tests/catch/unit/deviceLib/fp8_e8m0.cc Added comprehensive underflow test cases (1e-50, denorm_min, double::min, float::min cast to double) with correct expected values, and updated Catch2 matcher syntax

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@AidanBeltonS AidanBeltonS left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the patch and regression test. This looks good to me, thanks for catching this! One minor comment.

@AidanBeltonS
AidanBeltonS requested a review from b-sumner January 8, 2026 12:17
@sshlyapn

sshlyapn commented Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

@AidanBeltonS @b-sumner thanks for the review!
Btw, regarding the disabled tests - is there a plan to re‑enable it soon?

@sshlyapn
sshlyapn force-pushed the double_to_e8m0__convert_fix branch from a16170d to f48ea24 Compare January 19, 2026 06:48
@sshlyapn
sshlyapn force-pushed the double_to_e8m0__convert_fix branch from b67b36d to bc61454 Compare January 29, 2026 11:08
@geomin12

geomin12 commented Feb 9, 2026

Copy link
Copy Markdown
Contributor

Force merging as hip-test errors occur in various other places:

2026-02-06T20:58:14.3141786Z C:/home/runner/_work/rocm-systems/rocm-systems/projects/hip-tests/catch/unit/streamperthread/hipStreamPerThread_Basic.cc(60): FAILED:
2026-02-06T20:58:14.3141891Z   CHECK( hostMem[i] == (i + 1 + 5) )
2026-02-06T20:58:14.3141973Z with expansion:
2026-02-06T20:58:14.3142060Z   0 == 33432087 (0x1fe2217)

occurs here as well: https://github.com/ROCm/rocm-systems/actions/runs/21760049052/job/62785319334
Will open issue with hip-tests team

@geomin12
geomin12 merged commit 5d84cba into ROCm:develop Feb 9, 2026
15 of 20 checks passed
systems-assistant Bot pushed a commit to ROCm/clr that referenced this pull request Feb 9, 2026
 exp wraparound) (#2461)

Co-authored-by: Andrei Kochin <andrei.kochin@amd.com>
[rocm-systems] ROCm/rocm-systems#2461 (commit 5d84cba)
systems-assistant Bot pushed a commit to ROCm/hip-tests that referenced this pull request Feb 9, 2026
 exp wraparound) (#2461)

Co-authored-by: Andrei Kochin <andrei.kochin@amd.com>
[rocm-systems] ROCm/rocm-systems#2461 (commit 5d84cba)
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.

9 participants