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

Escape subnormal values #1953

Merged
merged 1 commit into from
May 30, 2024
Merged

Escape subnormal values #1953

merged 1 commit into from
May 30, 2024

Conversation

shiltian
Copy link
Contributor

Currently we don't escape subnormal values when generating image data. In sampler read tests, we use != to check the two values even when it is floating-point data, which requires the two values are bitwise equal. However, a sampler might flush subnormal values, causing the test case to fail.

In this patch, when generating random image data, we escape subnormal values.

@CLAassistant
Copy link

CLAassistant commented Apr 19, 2024

CLA assistant check
All committers have signed the CLA.

@b-sumner
Copy link
Contributor

Section 8.3.3 of the C spec specifically allows sampling to flush denorms, so this change seems to be long overdue.

@lakshmih
Copy link
Contributor

Reviewing

bashbaug
bashbaug previously approved these changes May 19, 2024
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.

I think I'm OK with this, but noting for completeness: this change removes all fp32 and fp16 denorm values from the input data. If we want to have some coverage for denorm inputs still, we will want to continue to include denorm inputs, and instead we will want to handle cases where denorms are preserved and when denorms are flushed to zero.

@bashbaug
Copy link
Contributor

A bit of spec archaeology, for the Khronos folks:

  • A fix similar to this one was proposed in Bugzilla 16324.
  • The spec changes were discussed in GitLab issues 30 and 113.

Currently we don't escape subnormal values when generating image data. In
sampler read tests, we use `!=` to check the two values even when it is
floating-point data, which requires the two values are bitwise equal. However, a
sampler might flush subnormal values, causing the test case to fail.

In this patch, when generating random image data, we escape subnormal values.
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.

Re-approving, looks like only whitespace / formatting changed.

@shiltian
Copy link
Contributor Author

I don't have privileges to merge the PR. Can someone do it for me? Thanks!

@bashbaug
Copy link
Contributor

It'd be nice to get one more approval before merging - @silverclaw or @lakshmih maybe?

If not, we'll aim to merge in our next teleconference, on Tuesday (June 4th).

Copy link
Contributor

@silverclaw silverclaw 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

Thanks! Merging.

@bashbaug bashbaug merged commit 556025b into KhronosGroup:main May 30, 2024
7 checks passed
@shiltian shiltian deleted the subnormal branch May 30, 2024 22:43
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.

6 participants