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

Сlarify rounding #2140

Closed
wants to merge 1 commit into from
Closed

Сlarify rounding #2140

wants to merge 1 commit into from

Conversation

munrocket
Copy link
Contributor

  • Solving inlined into spec issue (currently UB)
Issue 9. Default rounding mode is an implementation choice. Is that what we want?
  • Making WGSL more clear about a*b+c expression in GLSL way (Paragraph 4.7.1 Range and precision)

@w3cbot
Copy link

w3cbot commented Sep 24, 2021

munrocket marked as non substantive for IPR from ash-nazg.

@github-actions
Copy link
Contributor

Previews, as seen when this build job started (e98f618):
WebGPU | IDL
WGSL
Explainer

@@ -6292,7 +6292,7 @@ the following exceptions:
of NaNs and infinities.
* Implementations may ignore the sign of a zero.
That is, a zero with a positive sign may behave like a zero a with a negative sign, and vice versa.
* No rounding mode is specified.
* Rounding mode is round-to-nearest even, but implementation may truncate results to zero.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This sounds strange to me. So is this line normatively saying what the rounding is, or is it basically "implementation defined" still?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe change implementation -> hardware?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, I mean - this says there are 2 possible behaviors: rounding to even and truncation to zero. Aren't those conflicting?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose we can't guarantee round-to-nearest-even for all cases, because sometime it works like round to zero (or truncation in other words). DX11 have term hardware so probably it fit better here too. From DX11: "hardware is allowed to truncate results to 32-bit rather than perform round-to-nearest-even"

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I was thinking about round() builtin, not general rounding.
FYI, we can't talk about "hardware" directly in the spec.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, so this is some kind of trade off.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason no rounding mode was specified is because underlying implementations are unconstrained that way.

Are you trying to constrain it to exactly one of two possibilities: round-to-even and round-to-zero?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is just editorial PR. But I wanted to constraint it as a superset of Spir-V, MSL and DX. Or even better to specify that it is loosely round-to-nearest.

Firefox/Chrome is doing well and hope Safari will do it in the same way. But according to spec 2+2=666 is legit rounding and I think it is a good counterexample to fix this.

@kvark kvark requested a review from dneto0 September 28, 2021 15:44
@@ -6292,7 +6292,7 @@ the following exceptions:
of NaNs and infinities.
* Implementations may ignore the sign of a zero.
That is, a zero with a positive sign may behave like a zero a with a negative sign, and vice versa.
* No rounding mode is specified.
* Rounding mode is round-to-nearest even, but implementation may truncate results to zero.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The reason no rounding mode was specified is because underlying implementations are unconstrained that way.

Are you trying to constrain it to exactly one of two possibilities: round-to-even and round-to-zero?

@@ -6310,9 +6310,6 @@ The <dfn>correctly rounded</dfn> result of the operation for floating point type

</div>

That is, the result may be rounded up or down:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have to keep this.

@@ -6419,8 +6416,9 @@ However, the result may not be the same when computed in floating point.
The reassociated result may be inaccurate due to approximation, or may trigger
an overflow or NaN when computing intermediate results.

An implementation may reassociate and/or fuse operations if the optimization is
at least as accurate as the original formulation.
<dfn noexport>Fusing</dfn> occur when two floating-point operation performed in one step.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The example of multiply-then-add is s less general than the original statement.
Also, the original statement constrains accuracy. So I think this part of the change is not good.

wgsl/index.bs Show resolved Hide resolved
@munrocket munrocket closed this Sep 29, 2021
@kdashg
Copy link
Contributor

kdashg commented Sep 29, 2021

WGSL meeting minutes 2021-09-28
  • JG: Is this reasonable?
  • DN: It’s wishing for something we don’t have. We can’t really narrow it down below two rounding modes.
  • JG: Should we just list the two?
  • DN: Plausible? Would like to see data. Those are the most common two, but I’m not 100% sure if that’s enough.
  • Needs Investigation
  • DN to also review current PR

@munrocket
Copy link
Contributor Author

Thanks for review.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants