-
Couldn't load subscription status.
- Fork 802
Execution Tests: Long Vector - Dot fp32 precision #7801
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, can we update the commit description to include "the why"
…omething for corner cases like subnormals
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I haven't deeply reviewed the mathmatics of this, so if you'd like some more review of that you may need to solicit another review.
|
Yup, I asked Tex to do a sanity review on that for me.
…________________________________
From: Damyan Pepper ***@***.***>
Sent: Friday, October 24, 2025 9:29 AM
To: microsoft/DirectXShaderCompiler ***@***.***>
Cc: Alex Sepkowski ***@***.***>; Author ***@***.***>
Subject: Re: [microsoft/DirectXShaderCompiler] Execution Tests: Long Vector - Dot fp32 precision (PR #7801)
@damyanp approved this pull request.
LGTM, but I haven't deeply reviewed the mathmatics of this, so if you'd like some more review of that you may need to solicit another review.
—
Reply to this email directly, view it on GitHub<#7801 (review)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ABK4EW43ZZHBARQNU4HM2WL3ZJHYDAVCNFSM6AAAAACIRDH2TSVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTGNZXG44DCNRQGQ>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
| // Finally, compute and add in the ULP on our final sum of epsilons. | ||
| AbsoluteEpsilon += computeAbsoluteEpsilon<T>(AbsoluteEpsilon, ULPTolerance); | ||
|
|
||
| Op.ValidationConfig.Tolerance = static_cast<float>(AbsoluteEpsilon); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So absolute epsilons for double tests are expressed as float? Couldn't that be an issue for tests of very small values and denorms in double precision?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could update ValidationConfig to use double instead of float to solve this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ValidationConfig.Tolerance is used to pass into helpers in HLSLTestUtils that expect a float. So, I'd just have to update that to cast instead. Doesn't seem like a meaningful change.
For your first point I'm not sure. I'm not familiar enough. As-is the trig functions are the only other tests using a non-zero epsilon of 0.0008f. Is that still a concern? How small is very small? Should we approach this when we follow up with a wider set of inputs? If we have concerns around that I think I would rather follow up on them after implementing the rest of the pending exec tests.
Co-authored-by: Tex Riddell <[email protected]>
Co-authored-by: Tex Riddell <[email protected]>
…kow/DirectXShaderCompiler into user/alsepkow/DotPrecision
…kow/DirectXShaderCompiler into user/alsepkow/DotPrecision
Update Dot expected values to be computed using doubles to improve precision of the final result.
Additionally, this PR adds logic to compute an absolute epsilon tolerance for the final result of each Dot operation by summing absolute epsilons calculated on each multiply and addition in the dot product. The summation portion is re-ordered to a worst-case ordering when computing the absolute epsilon error tolerance as the DX spec allows for this.