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

AbstractFloat should not be convertible to infinity or NaN in concrete float type #2953

Closed
dneto0 opened this issue May 26, 2022 · 4 comments · Fixed by #2984
Closed

AbstractFloat should not be convertible to infinity or NaN in concrete float type #2953

dneto0 opened this issue May 26, 2022 · 4 comments · Fixed by #2984
Labels
wgsl WebGPU Shading Language Issues
Projects
Milestone

Comments

@dneto0
Copy link
Contributor

dneto0 commented May 26, 2022

1e100 is a valid AbstractFloat value, but outside the finite range of f32.

When converting a value from one float type to another, values outside the finite range of the target (modulo possible truncation of mantissa bits) are converted to an infinity of the target type.

So f32(1e100) should be the infinity of f32.

This is a bit weird:

  • you can't write a float literal with 'f' suffix that does the same thing. So f32(...) is not equivalent to the .....f form.
  • also, WGSL doesn't have reliable support for infinity (or NaN) because that reflects underlying implementation limitations. There is no isinf, isnan equivalent that can be reliably implemented.

So, we're proposing a change to the float conversion rules: When a float to float conversion would produce an infinity, but the source type is AbstractFloat (and hence a creation-time constant value), then that produces a shader creation error.

Credit @ben-clayton for finding this issue and proposing the solution.

@dneto0 dneto0 added the wgsl WebGPU Shading Language Issues label May 26, 2022
@dneto0 dneto0 added this to the V1.0 milestone May 26, 2022
@dneto0
Copy link
Contributor Author

dneto0 commented May 26, 2022

Also, this rule mirrors the rule that an AbstractInt value outside the range of a concrete integer type is not convertible to that type. It also produces a shader creation error. See https://gpuweb.github.io/gpuweb/wgsl/#ref-for-abstractint%E2%91%A1

@ben-clayton
Copy link
Collaborator

Should we also prevent hex floats from constructing a NaN?

@dneto0
Copy link
Contributor Author

dneto0 commented May 26, 2022

Yes.

For hex float literals, the spec says:

The value of the literal is the value of the mantissa multiplied by 2 to the power of the exponent. When no exponent is specified, an exponent of 0 is assumed.

A NaN occurs when the mantissa is nonzero and the biased exponent is the all 1's value for that field, or the maximum representable exponent. So putting a NaN bit representation through the ideal value calculation (quoted), the result value is outside the finite range of the concrete float type.

So the spec update is basically: It's a shader creation error if an AbstractFloat value is being converted to a concrete float type, but the value is outside the finite range of the target type.

@dneto0 dneto0 changed the title AbstractFloat should not be convertible to infinity in concrete float type AbstractFloat should not be convertible to infinity or NaN in concrete float type May 26, 2022
dneto0 added a commit to dneto0/gpuweb that referenced this issue May 31, 2022
@kdashg
Copy link
Contributor

kdashg commented May 31, 2022

WGSL meeting minutes 2022-05-31
  • DN: PR now posted: AbstractFloat to out-of-finite-range is shader creation error #2984
  • DN: When abstract numeric is too big for concrete type should be shader creation error. More in line with big float with f being shader creation error. Just makes it consistent.
  • KG: Is this normal for a language or a new thing?
  • DN: Normal thing I think.
  • KG: Often warning or error?
  • DN: Literal it’s an error. Looked at c++14, says if converting from double -> float (narrowing the type) if the value doesn’t fit in the finite range then it’s undefined behaviour. Don’t want implementation to do the check as it’s a hazard, so lets avoid.
  • KG: Works for me. Let’s take it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
wgsl WebGPU Shading Language Issues
Projects
WGSL
Awaiting triage
Development

Successfully merging a pull request may close this issue.

3 participants