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

Add additional i31ref spec tests #534

Merged
merged 2 commits into from
Apr 9, 2024

Conversation

fitzgen
Copy link
Contributor

@fitzgen fitzgen commented Apr 4, 2024

I noticed that Wasmtime was passing the spec tests despite having bits that were known to be unimplemented. This should help the tests exercise those corners of the spec.

Notably:

  • Setting i31ref globals.

  • Initializing tables and globals with (ref.i31 (global.get $g)).

  • Table operations on i31ref tables.

  • Accessing anyref globals and tables that are actually i31refs. This is interesting to exercise outside of general subtyping because we have different paths in our inline GC barriers for anyrefs that are actually i31refs.

@rossberg PTAL

I noticed that Wasmtime was passing the spec tests despite having bits that were
known to be unimplemented. This should help the tests exercise those corners of
the spec.

Notably:

* Setting `i31ref` globals.

* Initializing tables and globals with `(ref.i31 (global.get $g))`.

* Table operations on `i31ref` tables.

* Accessing `anyref` globals and tables that are actually `i31ref`s. This is
  interesting to exercise outside of general subtyping because we have different
  paths in our inline GC barriers for `anyref`s that are actually `i31ref`s.
Copy link
Member

@rossberg rossberg left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good. Should we also have non-nullable variants of these tests?

One nit: Can you match the folded style of the rest of this test?

@fitzgen
Copy link
Contributor Author

fitzgen commented Apr 8, 2024

Should we also have non-nullable variants of these tests?

There are a couple non-null types in there. Indeed, we avoid generating null-checks for GC barriers when we have non-null types. I think in an ideal world, yes, we would duplicate ~everything once for nullable and once more for non-nullable types. Don't really want to do that by hand though. Maybe if we were generating tests programmatically.

One nit: Can you match the folded style of the rest of this test?

Done!

@fitzgen
Copy link
Contributor Author

fitzgen commented Apr 9, 2024

(I don't have write access and can't merge this PR myself)

@rossberg rossberg merged commit e19c032 into WebAssembly:main Apr 9, 2024
1 check passed
@fitzgen
Copy link
Contributor Author

fitzgen commented Apr 9, 2024

Thanks!

@fitzgen fitzgen deleted the more-i31ref-spec-tests branch April 9, 2024 15:51
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.

2 participants