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

A few small wasm-smith index space refactorings #617

Merged

Conversation

fitzgen
Copy link
Member

@fitzgen fitzgen commented May 20, 2022

Trying to split things out of my big WIP patch that adds support for instantiating modules and components in wasm-smith.

There will be some more things along these lines soon, but they will also involve things like recursively copying a type definition into a new index space (needed for getting InstanceTypes and ComponentTypes of instantiations and components we generate) and are a bit more involved so I figured this was a good check point.

fitzgen added 4 commits May 19, 2022 16:03
Rather than represent the functions index space for components as an indirect
`(nth_section, nth_enty_in_section)` pair, just store the function type
directly.

Eventually, this will split into a components function index space and a core
function index space, and this change will make that easier.
Rather than an indirect list of `(nth_section, nth_entry_in_section)` pairs. Its
more direct, and easier to work with.
…ypes

Rather than an indirect list of `(nth_section, nth_entry_in_section)`
pairs. This is easier to work with and more direct.
First of all, this just makes working with these types easier, and isn't an
issue because they are immutable and we've already decided to generate them as
is.

Second, this helps us avoid issues where an aliased compound type will reference
other types by index and that index is in a different types index space than
where it is aliased from, and therefore it is easy to look up types in the wrong
index space. An alternative would be to keep track of which index space we are
talking about, but that is harder to work with than just having a reference to
the type itself on hand.
@fitzgen fitzgen requested a review from peterhuene May 20, 2022 21:30
This percolates down to some `wasm-encoder` types as well.
@peterhuene
Copy link
Member

FYI, we probably need to coordinate our upcoming changes as I'm fixing up what's currently in wasm-smith for a wasm-encoder refactoring resulting from this proposed change to the spec.

It'll definitely impact the alias and instance sections that you're currently implementing too (specifically, different index spaces and sections for core vs. component items).

I can put up a draft PR to show you what the changes currently look like.

I'll review this PR shortly as this looks easy enough to rebase my work on.

@fitzgen
Copy link
Member Author

fitzgen commented May 20, 2022

Yeah that's part of my motivation with splitting these things out from my bigger WIP PR: it should make the total amount of effort put into rebasing easier.

It also makes very clear in a few places where things will become two different index spaces (see the ComponentOrCoreFoo types, which will go away and we'll just ahve two different index spaces instead).

@peterhuene
Copy link
Member

Sneak peak of my spec update changes are here. That's just wasm-encoder, wasm-mutate, and wasm-smith (without rebasing on this PR yet).

I'm now working on updating wasmparser and then wast so I can get the component tests running again.

@fitzgen
Copy link
Member Author

fitzgen commented May 23, 2022

I'm going to go ahead and merge this PR, and then hold off on subsequent PRs until after your index space split stuff has landed.

@fitzgen fitzgen merged commit b2de804 into bytecodealliance:main May 23, 2022
@fitzgen fitzgen deleted the wasm-smith-index-space-refactorings branch May 23, 2022 16:04
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