Skip to content

Use values with smaller error for tan.16 - #1512

Merged
bogner merged 7 commits into
llvm:mainfrom
bogner:2026-09-10-tan16
Sep 12, 2026
Merged

Use values with smaller error for tan.16#1512
bogner merged 7 commits into
llvm:mainfrom
bogner:2026-09-10-tan16

Conversation

@bogner

@bogner bogner commented Sep 10, 2026

Copy link
Copy Markdown
Contributor

A few of the values here were a bit too likely to differ across APIs and hardware.

  • In practice I would expect most uses of tan (especially in half precision) to be on inputs between -2pi and 2pi, and values like 100.0h that are 10pi or more could easily hit a lot of intermediate rounding.
  • The 100pi input here is pretty far off from being representable in half (0x5ce9 is 314.25), so we're just testing another arbitrary value here that is also susceptible to rounding errors. Using the well-known 355/113 approximation of pi we can test something that's more likely to be consistent.
  • We didn't have a test for pi itself, which seems pretty useful.
  • An arbitrary number in the likely range is also a good idea, so I added cases for 2.0h.

A few of the values here were a bit too likely to differ across APIs and
hardware.

- In practice I would expect most uses of tan (especially in half
  precision) to be on inputs between -2pi and 2pi, and values like
  100.0h that are 10pi or more could easily hit a lot of intermediate
  rounding.
- The 100pi input here is pretty far off from being representable in
  half (0x5ce9 is 314.25), so we're just testing another arbitrary value
  here that is also susceptible to rounding errors. Using the well-known
  355/113 approximation of pi we can test something that's more likely
  to be consistent.
- We didn't have a test for pi itself, which seems pretty useful.
- An arbitrary number in the likely range is also a good idea, so I
  added cases for 2.0h.
@bogner
bogner requested a review from danbrown-amd September 10, 2026 19:40
@bogner bogner added the test-all When applied to a PR this will opt-in to additional pre-merge test configurations.. label Sep 10, 2026

@kmpeng kmpeng 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.

LGTM

Comment thread test/Feature/HLSLLib/tan.16.test Outdated

@bob80905 bob80905 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.

LGTM

Comment thread test/Feature/HLSLLib/tan.16.test
Comment thread test/Feature/HLSLLib/tan.16.test
Comment thread test/Feature/HLSLLib/tan.16.test Outdated
tan(2.0h) is 2 ULPs off on AMD. We could arguably bump this all the way up to 4
@bogner
bogner merged commit f5be1d1 into llvm:main Sep 12, 2026
42 of 55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test-all When applied to a PR this will opt-in to additional pre-merge test configurations..

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants