-
Notifications
You must be signed in to change notification settings - Fork 86
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
wgsl: Add AbstractFloat round
execution tests
#3080
Conversation
Previews, as seen when this build job started (1425a63): |
1425a63
to
5a0f388
Compare
Previews, as seen when this build job started (5a0f388): |
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.
I think the "large int value" case is not exercised the way it was intended.
abstract: () => { | ||
return FP.abstract.generateScalarToIntervalCases( | ||
[ | ||
reinterpretU64AsF64(0x8000_0000_0000_0000n), // https://github.com/gpuweb/cts/issues/2766 |
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.
This is a reinterpret cast, so this generates -0.0 in f64.
I don't think that's what you mean.
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.
The point of 2766 was to test integers that are larger in magnitude than can fit inside the mantissa bits. So for f64 that would be something in excess of 2** 53 ish.
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.
Correct, thanks for catching 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.
arg... I updated the unittests, but not the CTS test itself 😮💨
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.
Updated
5a0f388
to
ea04cd1
Compare
3029762
to
e66f317
Compare
Previews, as seen when this build job started (097b3d9): |
@dneto0 PTAL, I think this should be fixed now |
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.
Looks good to me, thanks
097b3d9
to
f34b35d
Compare
f34b35d
to
442ab96
Compare
I have addressed the concerns, but they are OOO for a week, so cannot confirm
Fixes #2509
Requirements for PR author:
.unimplemented()
./** documented */
and new helper files are found inhelper_index.txt
.Requirements for reviewer sign-off:
When landing this PR, be sure to make any necessary issue status updates.