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

Normative: add Float16Array and Math.f16round #3532

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

bakkot
Copy link
Contributor

@bakkot bakkot commented Feb 6, 2025

This adds the spec text from proposal-float16array. I intend to ask for stage 4 at the upcoming meeting.

@ljharb ljharb added normative change Affects behavior required to correctly evaluate some ECMAScript source text pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4. has test262 tests labels Feb 6, 2025
Copy link
Contributor

@syg syg left a comment

Choose a reason for hiding this comment

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

lgtm

<h1>DataView.prototype.getFloat16 ( _byteOffset_ [ , _littleEndian_ ] )</h1>
<p>This method performs the following steps when called:</p>
<emu-alg>
1. Let _v_ be the *this* value.
Copy link
Contributor

Choose a reason for hiding this comment

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

I guess v is short for "view" instead of "value", but still kinda weird we call the receiver v in DataView methods.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah GetViewValue/SetViewValue call this parameter view. The alias should match it here.

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 matches getFloat32. We can later change all of them but I don't want to introduce an inconsistency between this and the float32 case.

@@ -32339,6 +32355,23 @@ <h1>Math.floor ( _x_ )</h1>
</emu-note>
</emu-clause>

<emu-clause id="sec-math.f16round">
Copy link
Collaborator

Choose a reason for hiding this comment

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

This puts f16round between floor and fround, which isn't lexicographic order.

ASCII order would put digits before letters (f16round before floor), but the few examples where it makes a difference in the spec collate digits after letters (atanh before atan2, log1p before log10), suggesting that f16round should be after fround.

Copy link
Member

Choose a reason for hiding this comment

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

As long as it's adjacent to fround, either side is fine by me.

1. Return the ECMAScript Number value corresponding to _n64_.
</emu-alg>
<emu-note>
<p>This operation is not the same as casting to binary32 and then to binary16 because of the possibility of double-rounding: consider the number _k_ = *1.00048828125000022204*<sub>𝔽</sub>, for example, for which which Math.f16round(_k_) is *1.0009765625*<sub>𝔽</sub>, but Math.f16round(Math.fround(_k_)) is *1*<sub>𝔽</sub>.</p>
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<p>This operation is not the same as casting to binary32 and then to binary16 because of the possibility of double-rounding: consider the number _k_ = *1.00048828125000022204*<sub>𝔽</sub>, for example, for which which Math.f16round(_k_) is *1.0009765625*<sub>𝔽</sub>, but Math.f16round(Math.fround(_k_)) is *1*<sub>𝔽</sub>.</p>
<p>This operation is not the same as casting to binary32 and then to binary16 because of the possibility of double-rounding: consider the number _k_ = *1.00048828125000022204*<sub>𝔽</sub>, for example, for which Math.f16round(_k_) is *1.0009765625*<sub>𝔽</sub>, but Math.f16round(Math.fround(_k_)) is *1*<sub>𝔽</sub>.</p>

@@ -43695,6 +43745,10 @@ <h1>
<emu-alg>
1. Let _elementSize_ be the Element Size value specified in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Element Type _type_.
1. If _isLittleEndian_ is *false*, reverse the order of the elements of _rawBytes_.
1. If _type_ is ~float16~, then
1. Let _value_ be the byte elements of _rawBytes_ concatenated and interpreted as a little-endian bit string encoding of an IEEE 754-2019 binary16 value.
1. If _value_ is an IEEE 754-2019 binary16 NaN value, return the *NaN* Number value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. If _value_ is an IEEE 754-2019 binary16 NaN value, return the *NaN* Number value.
1. If _value_ is an IEEE 754-2019 binary16 NaN value, return *NaN*.

I don't see a reason to stray from our normal editorial conventions here.

Copy link
Contributor Author

@bakkot bakkot Feb 15, 2025

Choose a reason for hiding this comment

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

This currently matches the phrasing in the ~float32~ branch immediately below, which seems more important. We can later change all of them to match but I don't want to introduce an inconsistency within this method.

@@ -43695,6 +43745,10 @@ <h1>
<emu-alg>
1. Let _elementSize_ be the Element Size value specified in <emu-xref href="#table-the-typedarray-constructors"></emu-xref> for Element Type _type_.
1. If _isLittleEndian_ is *false*, reverse the order of the elements of _rawBytes_.
1. If _type_ is ~float16~, then
1. Let _value_ be the byte elements of _rawBytes_ concatenated and interpreted as a little-endian bit string encoding of an IEEE 754-2019 binary16 value.
1. If _value_ is an IEEE 754-2019 binary16 NaN value, return the *NaN* Number value.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
1. If _value_ is an IEEE 754-2019 binary16 NaN value, return the *NaN* Number value.
1. If _value_ is an IEEE 754-2019 binary16 format Not-a-Number encoding, return the *NaN* Number value.

for consistency with phasing in NumericToRawBytes. Specifically, I'd prefer not to use "NaN" to refer to anything other than a binary64 NaN.

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 currently matches the phrasing in the ~float32~ branch immediately below, which seems more important. We can later change all of them to match but I don't want to introduce an inconsistency within this method.

Copy link
Member

Choose a reason for hiding this comment

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

We could also just make a quick PR addressing these editorial concerns and rebase your proposal PR on top of it.

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 don't think it makes sense to care about rushing the editorial change in before landing this PR given that this exact wording has stood for most of a decade. I'd prefer to land this in a form consistent with the existing spec and then bikeshed any other editorial changes to the surrounding stuff at our leisure.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with @bakkot, consistency for now, debating wording changes after.

Copy link
Member

@michaelficarra michaelficarra left a comment

Choose a reason for hiding this comment

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

LGTM otherwise

@bakkot bakkot added has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. and removed pending stage 4 This proposal has not yet achieved stage 4, but may otherwise be ready to merge. labels Feb 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
has stage 4 This PR represents a proposal that has achieved stage 4, and is ready to merge. has test262 tests normative change Affects behavior required to correctly evaluate some ECMAScript source text proposal This is related to a specific proposal, and will be closed/merged when the proposal reaches stage 4.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants