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

Define non-trapping float-to-int conversions. #1089

Merged
merged 5 commits into from
Feb 27, 2020

Conversation

sunfishcode
Copy link
Member

This also introduces the concept of prefix bytes, and defines the "numeric" prefix byte, used for encoding the new conversion instructions.

As far as I'm aware, this proposal is fully in accord with the CG guidance expressed at the recent CG meeting.

Copy link
Member

@jfbastien jfbastien left a comment

Choose a reason for hiding this comment

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

Please identify all of the new things with a "feature marker" of some sort, as we've done for the stream APIs (some Unicode identifier). That way post-MVP features are clear.

For this I suggest the wording I had here:
https://github.com/WebAssembly/design/pull/1027/files

Otherwise I do believe this is what the CG voted for, so it lgtm.

@sunfishcode
Copy link
Member Author

Ok, I added 🎳 as feature marker for this feature. Does this look right?

@jfbastien
Copy link
Member

Ok, I added 🎳 as feature marker for this feature. Does this look right?

Does it looks good?!?! IT LOOKS AMAZING.

Still lgtm, but it would be great to get Goog / MS to sign off on this. @flagxor / @MikeHolman ?

@jfbastien jfbastien requested review from MikeHolman and flagxor June 9, 2017 21:59
@juj
Copy link

juj commented Jun 15, 2017

This is superb!

How does one feature test for the presence of additions like this?

@sunfishcode
Copy link
Member Author

One can construct minimal WebAssembly modules and test whether they validate, for example using WebAssembly.validate in the JS API.

For example:

var t = new Uint8Array([0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00,
                        0x01, 0x06, 0x01, 0x60, 0x01, 0x7d, 0x01, 0x7f,
                        0x03, 0x02, 0x01, 0x00, 0x0a, 0x08, 0x01, 0x06,
                        0x00, 0x20, 0x00, 0xfc, 0x00, 0x0b]);

if (WebAssembly.validate(t)) {
  // i32.trunc_s:sat/f32 is supported.
}

@juj
Copy link

juj commented Jun 15, 2017

That makes sense, thanks!

@lars-t-hansen
Copy link
Contributor

@sunfishcode, somehow that proposal does not seem quite satisfactory to me. I know the version number is to stay at 1 "forever" but if we could guarantee that then there would be no version number to begin with. We should therefore expect that the version number will eventually have to change, notably if a feature has to be changed or removed because it turns out to be unsafe; eventually implementations may then stop accepting old content. Ergo we may in time see a situation where the above code no longer validates, not because the feature is not supported, but because the version is not accepted.

Technically, the snipped above tests not whether the feature is available, but whether it's available in a version 1 module.

Presumably the test above could attempt to validate two modules, one empty module with version one and then the module with the feature. It might be useful to add a method, WebAssembly.acceptsVersion(k), to highlight this issue?

@sunfishcode
Copy link
Member Author

@lars-t-hansen That's true. The above follows the path envisioned when has_feature was removed. I will seek input from others.

Currently, to test just version 1 from JS, one can do this:

var t = new Uint8Array([0x00, 0x61, 0x73, 0x6d, 0x01, 0x00, 0x00, 0x00]);

if (WebAssembly.validate(t)) {
  // Version 1 is supported.
}

@lukewagner
Copy link
Member

@lars-t-hansen The version is indeed intended to stay at 1 "forever". Instead of bumping version, we can continue to add new: section ids (and thus section-body formats), import and export kinds (to the existing import/export sections), and types and operators (to the existing code section). All of these can be added by just accepting new bit patterns to existing fields that were previously rejected. This is not fundamentally different than JS adding new syntactic forms over time that you have to test via try { eval("function*(){}") }.

We should therefore expect that the version number will eventually have to change, notably if a feature has to be changed or removed because it turns out to be unsafe; eventually implementations may then stop accepting old content.

If a feature had to be removed, the associated opcodes/typecodes/sectionids/etc would just begin to fail validation; that doesn't require a version bump. (JS and the Web do this all the time without bumping versions.) The only question is if the removed bit patterns stay reserved forever or one day get "recycled".

Really, the version word is just an escape hatch if it becomes evident in some number of years that there is a much better binary format that we'd like to support (in addition to version=1 format, of course) or just some terribly fundamental flaw we need to fix. The bar for this is extremely high, though.

@jfbastien
Copy link
Member

It would be helpful if someone volunteered to update https://github.com/WebAssembly/design/blob/master/FeatureTest.md 😀

@jfbastien
Copy link
Member

Can someone from Google please confirm that this is good to go?

Copy link
Member

@binji binji left a comment

Choose a reason for hiding this comment

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

lgtm, do you need someone with more clout? :-)

@jfbastien
Copy link
Member

No, I just want to make sure all implementors sign off. It's between you, @flagxor, and friends who between you can sign off 😄

@sunfishcode sunfishcode force-pushed the nontrapping-conversions branch from e689eb7 to 88625a7 Compare August 30, 2019 05:27
@sunfishcode
Copy link
Member Author

Rebased on master, updated opcodes to reflect the new names, renamed "numeric" to "misc".

@sunfishcode
Copy link
Member Author

Non-trapping float-to-int conversions have progressed to phase 4, and this design-repo PR has multiple approvals and no outstanding objections, so let's merge this!

I know some of these design-repo docs aren't active in general, but I mainly just want to merge this to close out this PR :-).

@sunfishcode sunfishcode merged commit ccbac15 into master Feb 27, 2020
@sunfishcode sunfishcode deleted the nontrapping-conversions branch February 27, 2020 02:30
@Abbesvensk

This comment was marked as off-topic.

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.

9 participants