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.

Sign up for GitHub

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

Jump to bottom

Add support for reference types proposal #2451

Merged
merged 23 commits into from
Dec 31, 2019
Merged

Add support for reference types proposal #2451

merged 23 commits into from
Dec 31, 2019

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Nov 19, 2019 •
edited
Loading

This adds support for the reference type proposal. This adds support for
all reference types (anyref, funcref(=anyfunc), and nullref) and
four new instructions: ref.null, ref.is_null, ref.func, and new
typed select. This also adds subtype relationship support between
reference types.

This does not include table instructions yet. This also does not include
wasm2js support.

Fixes #2444 and fixes #2447.

@MaxGraey MaxGraey mentioned this pull request Nov 19, 2019
Closed
@aheejin aheejin force-pushed the reference_types branch 2 times, most recently from a96d002 to fe7d2b2 Compare November 30, 2019 02:34
aheejin added a commit to aheejin/binaryen that referenced this pull request Nov 30, 2019
@aheejin
Move type utility classes to type-utils.h (NFC)
b46e447 
This renames type-updating.h to type-utils.h and moves TypeSeeker class,
which is currently only used in wasm.cpp, to that file too. I plan to
use this elsewhere in WebAssembly#2451 and I think making this a separate PR makes
things easier to review.
@aheejin aheejin mentioned this pull request Nov 30, 2019
Closed
aheejin added a commit to aheejin/binaryen that referenced this pull request Nov 30, 2019
@aheejin
Move type utility classes to type-utils.h (NFC)
acfe366 
This renames type-updating.h to type-utils.h and moves `TypeSeeker`
class, which is currently only used in wasm.cpp, to that file too. I
plan to use this elsewhere in WebAssembly#2451 and I think making this a separate
PR makes things easier to review.
@aheejin
Copy link
Member Author

aheejin commented Dec 2, 2019

This is NOT ready yet... I'll mark this as non-draft once it is.

@aheejin aheejin force-pushed the reference_types branch 8 times, most recently from 3359a91 to 12b15e7 Compare December 6, 2019 04:37
aheejin added a commit to aheejin/binaryen that referenced this pull request Dec 6, 2019
@aheejin
Make local.tee's type its local's type
be7b6a0 
According to the current spec, `local.tee`'s return type should be the
same as its local's type. But currently in Binaryen `local.tee`'s type
is computed from its value's type. This didn't make any difference in
the MVP, but after we have subtype relationship in WebAssembly#2451, this can
become a problem. For example:
```
(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)
```
This shouldn't validate in the spec, but this will pass Binaryen
validation with the current `local.tee` implementation.

This makes `local.tee`'s type computed from the local's type, and makes
`LocalSet::makeTee` get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class `LocalSet` because it may increase memory size.

This also fixes the type of `local.get` to be the local type where
`local.get` and `local.set` pair is created from `local.tee`.
aheejin added a commit to aheejin/binaryen that referenced this pull request Dec 6, 2019
@aheejin
Make local.tee's type its local's type
b873441 
According to the current spec, `local.tee`'s return type should be the
same as its local's type. (Discussions on whether we should change this
rule is going on in WebAssembly/reference-types#55, but here I will
assume this spec does not change. If this changes, we should change many
parts of Binaryen transformation anyway...)

But currently in Binaryen `local.tee`'s type is computed from its
value's type. This didn't make any difference in the MVP, but after we
have subtype relationship in WebAssembly#2451, this can become a problem. For
example:
```
(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)
```
This shouldn't validate in the spec, but this will pass Binaryen
validation with the current `local.tee` implementation.

This makes `local.tee`'s type computed from the local's type, and makes
`LocalSet::makeTee` get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class `LocalSet` because it may increase memory size.

This also fixes the type of `local.get` to be the local type where
`local.get` and `local.set` pair is created from `local.tee`.
aheejin added a commit to aheejin/binaryen that referenced this pull request Dec 6, 2019
@aheejin
Make local.tee's type its local's type
dad5c60 
According to the current spec, `local.tee`'s return type should be the
same as its local's type. (Discussions on whether we should change this
rule is going on in WebAssembly/reference-types#55, but here I will
assume this spec does not change. If this changes, we should change many
parts of Binaryen transformation anyway...)

But currently in Binaryen `local.tee`'s type is computed from its
value's type. This didn't make any difference in the MVP, but after we
have subtype relationship in WebAssembly#2451, this can become a problem. For
example:
```
(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)
```
This shouldn't validate in the spec, but this will pass Binaryen
validation with the current `local.tee` implementation.

This makes `local.tee`'s type computed from the local's type, and makes
`LocalSet::makeTee` get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class `LocalSet` because it may increase memory size.

This also fixes the type of `local.get` to be the local type where
`local.get` and `local.set` pair is created from `local.tee`.
@aheejin aheejin mentioned this pull request Dec 6, 2019
Merged
aheejin added a commit to aheejin/binaryen that referenced this pull request Dec 8, 2019
@aheejin
Make local.tee's type its local's type
dd78189 
According to the current spec, `local.tee`'s return type should be the
same as its local's type. (Discussions on whether we should change this
rule is going on in WebAssembly/reference-types#55, but here I will
assume this spec does not change. If this changes, we should change many
parts of Binaryen transformation anyway...)

But currently in Binaryen `local.tee`'s type is computed from its
value's type. This didn't make any difference in the MVP, but after we
have subtype relationship in WebAssembly#2451, this can become a problem. For
example:
```
(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)
```
This shouldn't validate in the spec, but this will pass Binaryen
validation with the current `local.tee` implementation.

This makes `local.tee`'s type computed from the local's type, and makes
`LocalSet::makeTee` get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class `LocalSet` because it may increase memory size.

This also fixes the type of `local.get` to be the local type where
`local.get` and `local.set` pair is created from `local.tee`.
aheejin added a commit to aheejin/binaryen that referenced this pull request Dec 10, 2019
@aheejin
Make local.tee's type its local's type
9dfa661 
According to the current spec, `local.tee`'s return type should be the
same as its local's type. (Discussions on whether we should change this
rule is going on in WebAssembly/reference-types#55, but here I will
assume this spec does not change. If this changes, we should change many
parts of Binaryen transformation anyway...)

But currently in Binaryen `local.tee`'s type is computed from its
value's type. This didn't make any difference in the MVP, but after we
have subtype relationship in WebAssembly#2451, this can become a problem. For
example:
```
(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)
```
This shouldn't validate in the spec, but this will pass Binaryen
validation with the current `local.tee` implementation.

This makes `local.tee`'s type computed from the local's type, and makes
`LocalSet::makeTee` get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class `LocalSet` because it may increase memory size.

This also fixes the type of `local.get` to be the local type where
`local.get` and `local.set` pair is created from `local.tee`.
@aheejin aheejin force-pushed the reference_types branch 2 times, most recently from 5b7d015 to 4ff8de8 Compare December 10, 2019 19:19
aheejin added a commit that referenced this pull request Dec 13, 2019
@aheejin
Make local.tee's type its local's type (#2511)
89d1cf9 
According to the current spec, `local.tee`'s return type should be the
same as its local's type. (Discussions on whether we should change this
rule is going on in WebAssembly/reference-types#55, but here I will
assume this spec does not change. If this changes, we should change many
parts of Binaryen transformation anyway...)

But currently in Binaryen `local.tee`'s type is computed from its
value's type. This didn't make any difference in the MVP, but after we
have subtype relationship in #2451, this can become a problem. For
example:
```
(func $test (result funcref) (local $0 anyref)
  (local.tee $0
    (ref.func $test)
  )
)
```
This shouldn't validate in the spec, but this will pass Binaryen
validation with the current `local.tee` implementation.

This makes `local.tee`'s type computed from the local's type, and makes
`LocalSet::makeTee` get a type parameter, to which we should pass the
its corresponding local's type. We don't embed the local type in the
class `LocalSet` because it may increase memory size.

This also fixes the type of `local.get` to be the local type where
`local.get` and `local.set` pair is created from `local.tee`.
@aheejin
Merge branch 'type_method_refactor' into reference_types
e7ea3d5
@aheejin
Move Type-related functions into Type class (NFC)
944ae89 
Several type-related functions currently exist outside of `Type`
class and thus in the `wasm`, effectively global, namespace. This moves
these functions into `Type` class, making them either member functions
or static functions.

Also this renames `getSize` to `getByteSize` to make it not to be
confused with `size`, which returns the number of types in multiple
types. This also reorders the order of functions in `wasm-type.cpp` to
match that of `wasm-type.h`.
@aheejin
Merge branch 'type_method_refactor' into reference_types
893ce13
@aheejin
Address more comments
5a0d4b4
@aheejin
Copy link
Member Author

aheejin commented Dec 25, 2019

The previous run of the fuzzer lasted ~87000 iterations, at which point I stopped the execution. Now I've made some changes while addressing the comments, I'll keep running the fuzzer on the new code.

kripken
kripken approved these changes Dec 26, 2019
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

With the Name change in Literal, this lgtm to land, the other stuff can be left for discussion later.

@aheejin
Copy link
Member Author

aheejin commented Dec 27, 2019

Thanks! This depends on #2556, so this can land after that.

@aheejin
Copy link
Member Author

aheejin commented Dec 27, 2019 •
edited
Loading

The fuzzer ran for more than 100000 iterations, after which it failed due to #2558, so I guess it's pretty safe now. #2558 is not related to reference types.

@aheejin
Copy link
Member Author

aheejin commented Dec 31, 2019

I'll merge this, because it's pending for a long time now. If other reviewers have comments, please leave them and I'll address them in follow-up PRs. Thanks!

@aheejin aheejin merged commit bcc7614 into WebAssembly:master Dec 31, 2019
@aheejin aheejin deleted the reference_types branch December 31, 2019 01:55
@dschuff
Copy link
Member

dschuff commented Jan 6, 2020

🎉

@kripken kripken mentioned this pull request Jan 6, 2020
Closed
@kripken kripken mentioned this pull request Jan 23, 2020
Merged
kripken added a commit that referenced this pull request Jan 23, 2020
@kripken
Initialize the LegalizeJSInterface vector once, not once in each func…
d6d5655 
…tion (#2614)

I missed this in the review of #2451 - this was doing quadratic
work, each function touched the entire array which is the size
of the functions.

This speeds up the pspdfkit testcase from the mailing list from
several minutes (15 on CI; I stopped measuring after 2 minutes
locally) to 5 seconds. I suspect this was not noticed earlier because
that testcase has a very large number of functions, which
hit this issue especially hard.
@TerrorJack TerrorJack mentioned this pull request May 9, 2020
Merged
@aheejin aheejin mentioned this pull request Sep 12, 2020
Merged
@martianboy martianboy mentioned this pull request Jan 23, 2021
Closed
@aheejin aheejin mentioned this pull request Dec 9, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kripken kripken kripken approved these changes

@tlively tlively Awaiting requested review from tlively

@dschuff dschuff Awaiting requested review from dschuff

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
3 participants
@aheejin @dschuff @kripken