-
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: Convert quantizeToI32/U32
to use Math.trunc
#3120
Conversation
src/webgpu/util/math.ts
Outdated
if (num <= kValue.i32.negative.min) { | ||
return kValue.i32.negative.min; | ||
} | ||
return Math.round(num); |
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.
Despite the function's comment, quantizeToI32
wasn't rounding to closest, but was rounding towards zero:
const F = 0.7;
const quantizeToI32Data = new Int32Array(new ArrayBuffer(4));
quantizeToI32Data[0] = F;
var message = `${quantizeToI32Data[0]}, ${Math.round(F)}`;
console.log(message)
This is a change of behaviour. Have you checked that all the places that call this expect the new behaviour?
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.
Changed these to use Math.trunc to replicate the behaviour. I have run all of the tests under webgpu:shader,execution,expression,* and the unittests, so I think this should be fine.
Another small bump (~5%) to be gained through using a builtin instead of trampolining through a TypedArray.
a82d059
to
0b436a0
Compare
quantizeToI32/U32
to used Math.round
quantizeToI32/U32
to use Math.trunc
The spec said On the other hand, if all tests are passed no matter using |
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.
Approving this change, but I agree with:
On the other hand, if all tests are passed no matter using
Math.round
orMath.trunc
, does it imply that some tests on this fp-int conversion is missing?
It sounds like we could do with some more unit tests, and probably some more CTS cases.
Looking at the call sites of these functions, since it has been awhile since I worked with this code. They are only being used as filtering/cleanup of initial input values, i.e. I want you to run CTS on this set of numbers, and these functions are used to fix up the values to be the correct type of integer. They are not being used to adjust things internally in the framework to make sure that results comply with the WGSL rounding rules. This is why the specific rounding rule doesn't affect if the test is passing, changing it just changes the set of inputs being tested on, not the expected result for a specific value. |
Another small bump (~5%) to be gained through using a builtin instead of trampolining through a TypedArray.
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.