-
Notifications
You must be signed in to change notification settings - Fork 314
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
Add scalar value constructors from abstracts #4449
Conversation
Fixes gpuweb#4415 * Add zero values for abstract numerics * Add overloads for scalar value constructors from abstract types for: * `bool` - NFC * `i32` - avoids conversion to f32 first * `f32` - allows `0xffffffff` to f32 * `f16` - allows `0xffffffff` to f16 * `u32` - avoids conversion to f32 first
Previews, as seen when this build job started (06721cb): |
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.
line 11975 says this:
Note: There are no direct conversions from [=AbstractFloat=] to an integer scalar type.
All conversions first convert to another floating point type ([=f32=] usually).
should that be removed @alan-baker with these changes?
Good catch, I'll remove that. |
Thank you, the engineer who brought this to my attention also noticed that wording :) |
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.
Yep. LGTM.
Thank you!
@@ -12382,14 +12385,15 @@ specify the component type; the component type is inferred from the constructor | |||
<td> | |||
<xmp highlight=wgsl>@const @must_use fn i32(e : T) -> i32</xmp> | |||
<tr><td>Parameterization | |||
<td>`T` is a [=type/concrete=] [=scalar=] type | |||
<td>`T` is a [=scalar=] type | |||
<tr><td>Description | |||
<td>Construct an [=i32=] value. | |||
|
|||
If `T` is [=i32=], this is an identity operation.<br> | |||
If `T` is [=u32=], this is a reinterpretation of bits (i.e. the result is the unique value in [=i32=] that has the same bit pattern as `e`).<br> | |||
If `T` is a [[#floating-point-types|floating point type]], `e` is [=scalar floating point to integral conversion|converted=] to [=i32=], rounding towards zero.<br> |
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.
Note to myself: AbstractFloat is now captured by this case directly. Before, we would use the automatic conversion AbstractFloat --> f32 and then use the rule for f32.
This implies the float number has to keep all 31 mantissa bits when constructing an i32.
for example
const big0 = 0x40000000.0;
const big1 = 0x40000001.0;
const big2 = 0x40000002.0; // these differ only in the 31st bit.
const_assert( i32(big0) < i32(big1)); // true
const_assert( i32(big1) < i32(big2)); // true
@@ -12837,7 +12838,7 @@ specify the component type; the component type is inferred from the constructor | |||
<td> | |||
<xmp highlight=wgsl>@const @must_use fn u32(e : T) -> u32</xmp> | |||
<tr><td>Parameterization | |||
<td>`T` is a [=type/concrete=] [=scalar=] type or [=AbstractInt=] |
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.
Similar comment here as above.
We can test the preservation of all 32 mantissa bits.
Thinking about this: f16 - allows 0xffffffff to f16 The max finite f16 value is 65504. See https://float.exposed/0x7bff So this might not be a functional change for f16. |
* `u32()` is 0 | ||
* `f32()` is 0.0 | ||
* `f16()` is 0.0 | ||
* `i32()` is 0i |
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.
Use backticks on the constant to match bool? (here and below)
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.
We're not particularly consistent in how we do this in the spec. I'll file a separate issue to try and clean up the spec more generally in this regard. I am in favour of using code/backticks for actual values though.
* The zero value for an *N*-component vector of type *T* is the *N*-component vector of the zero value for *T*. | ||
* The zero value for an *C*-column *R*-row matrix of type *T* is the matrix of those dimensions filled with the zero value for *T*. | ||
* The zero value for a [=constructible=] *N*-element array with element type *E* is an array of *N* elements of the zero value for *E*. | ||
* The zero value for a [=constructible=] structure type *S* is the structure value *S* with zero-valued members. | ||
* The zero value of an [=AbstractInt=] is 0. |
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.
Backticks on the 0
would help separate the full-stop from the literal (here and below)
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.
Will be fixed with #4485
https://bugs.webkit.org/show_bug.cgi?id=267336 rdar://120783899 Reviewed by Mike Wyrzykowski. There were several CTS tests failing because they relied on directly converting abstract floats to concrete integers. This behavior didn't match the spec, but the spec is now being updated to match the CTS instead[1]. [1]: gpuweb/gpuweb#4449 * Source/WebGPU/WGSL/ConstantFunctions.h: (WGSL::CONSTANT_FUNCTION): * Source/WebGPU/WGSL/TypeDeclarations.rb: Canonical link: https://commits.webkit.org/272894@main
Fixes #4415
bool
- NFCi32
- avoids conversion to f32 firstf32
- allows0xffffffff
to f32f16
- allows0xffffffff
to f16u32
- avoids conversion to f32 first