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

wasm-metadce all the things #6142

Merged
merged 36 commits into from
Nov 30, 2023
Merged

wasm-metadce all the things #6142

merged 36 commits into from
Nov 30, 2023

Conversation

kripken
Copy link
Member

@kripken kripken commented Nov 30, 2023

Remove hardcoded paths for globals/functions/etc. in favor of general code
paths that support all the module elements uniformly. As a result of that, we
now support all parts of wasm, such as tables and element segments, that
we didn't before.

This refactoring is NFC aside from adding functionality. Note that this reduces
the size of wasm-metadce by 10% while increasing its functionality - the
benefits of writing generic code.

To support this, add some trivial generic helpers to get or iterate over module
elements using their kind in a dynamic manner. Using them might make
wasm-metadce slightly slower, but I can't measure any difference.

@kripken kripken requested a review from tlively November 30, 2023 20:50
@kripken
Copy link
Member Author

kripken commented Nov 30, 2023

Side note, with this metadce should be future-proof for things like adding Events to wasm.

@tlively
Copy link
Member

tlively commented Nov 30, 2023

This refactoring is NFC aside from adding functionality.

😆

scanner.setModule(&wasm);
scanner.walk(global->init);
});
// we can't remove segments, so root what they need
// We can't remove active segments, so root them and what they use.
Copy link
Member

Choose a reason for hiding this comment

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

In principle we could remove them if their associated memory or table is removed. They form a little cycle.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense, added a TODO.

Comment on lines 1504 to 1505
default:
WASM_UNREACHABLE("invalid kind");
Copy link
Member

Choose a reason for hiding this comment

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

If you put the WASM_UNREACHABLE after the switch instead of having a default case, the compiler will help us out by complaining if we add a new ModuleItemKind without handling it here.

Copy link
Member Author

Choose a reason for hiding this comment

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

Good idea, done.

@kripken kripken merged commit 42cddbf into main Nov 30, 2023
14 checks passed
@kripken kripken deleted the meta.all branch November 30, 2023 23:16
radekdoulik pushed a commit to dotnet/binaryen that referenced this pull request Jul 12, 2024
Remove hardcoded paths for globals/functions/etc. in favor of general code
paths that support all the module elements uniformly. As a result of that, we
now support all parts of wasm, such as tables and element segments, that
we didn't before.

This refactoring is NFC aside from adding functionality. Note that this reduces
the size of wasm-metadce by 10% while increasing its functionality - the
benefits of writing generic code.

To support this, add some trivial generic helpers to get or iterate over module
elements using their kind in a dynamic manner. Using them might make
wasm-metadce slightly slower, but I can't measure any difference.
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