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

spirv_new: fix test_decorate to use the device's default rounding #1987

Merged
merged 4 commits into from
Jul 2, 2024

Conversation

cycheng
Copy link
Contributor

@cycheng cycheng commented Jun 20, 2024

The verification code assumes the hardware uses CL_HALF_RTE, which causes a mismatch computation results when the hardware uses RTZ. Fix to use the hardware's default rounding mode.

@CLAassistant
Copy link

CLAassistant commented Jun 20, 2024

CLA assistant check
All committers have signed the CLA.

@svenvh
Copy link
Member

svenvh commented Jun 20, 2024

@cycheng there is a related fix #1980 currently under review. Any chance that solves the issue for your use case too?

@cycheng
Copy link
Contributor Author

cycheng commented Jun 20, 2024

@svenvh , I am afraid not because we see the mismatch in

f = cl_half_to_float(cl_half_from_float(f, half_rounding));

The inputs are: 0xD6F0 (-111.000000) and 0x3B91 (0.945801)
f = -111.0 * 0.945801
RTE hw produces: -105.000000
RTZ hw produces: -104.983887

But I am okay to drop this pull request if #1980 can merge my changes.

@svenvh
Copy link
Member

svenvh commented Jun 20, 2024

But I am okay to drop this pull request if #1980 can merge my changes.

Not sure what @hvdijk thinks about that, but we can also first land #1980 without any changes, and then you can rebase your changes on top. They seem to be separate issues anyway.

@hvdijk
Copy link
Contributor

hvdijk commented Jun 20, 2024

I agree, they look like separate issues to me too, but I'm happy with whatever you prefer, update my PR to include this, update this PR to include mine, or keep them separate, just let me know if I need to do anything.

@cycheng
Copy link
Contributor Author

cycheng commented Jun 20, 2024

@hvdijk , @svenvh, I am happy to close this pull request and let @hvdijk merge this change into #1980. :)

@cycheng cycheng force-pushed the spirv_new_decorate_half_rtz branch from 9054052 to adf1ab6 Compare June 26, 2024 05:08
@neiltrevett neiltrevett merged commit 1cd0266 into KhronosGroup:main Jul 2, 2024
7 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.

6 participants