Skip to content

[1/x] Add Wasm component translation support to the integration tests #127

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

Merged
merged 2 commits into from
Mar 15, 2024

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented Feb 12, 2024

Add Wasm component translation support to CompileTest.
Add a text representation of the IR Component and use it in the expected tests.

@greenhat greenhat force-pushed the greenhat/i107-cargo-comp-v0.7 branch from 7bc8936 to c46ca5d Compare February 15, 2024 08:32
@greenhat greenhat force-pushed the greenhat/wasmcm-itest-support branch from 5252664 to 30cb7dd Compare February 15, 2024 16:12
Base automatically changed from greenhat/i107-cargo-comp-v0.7 to main February 15, 2024 22:48
@greenhat
Copy link
Contributor Author

@bitwalker I'd like Component to have a textual representation for expected tests. I'm thinking of putting module's body in curly braces (+indent) and printing them sequentially. Plus, I'd like to print Component imports as well. So far I drafted them like https://github.com/0xPolygonMiden/compiler/blob/72bf0d30e69f856e2313ff8f9e5701dfc359e372/tests/integration/expected/components/inc_wasm_component.hir#L3 .
What do you think?

@greenhat greenhat force-pushed the greenhat/wasmcm-itest-support branch from 72bf0d3 to 30cb7dd Compare February 16, 2024 13:47
@bitwalker
Copy link
Contributor

I'm thinking of putting module's body in curly braces (+indent) and printing them sequentially

I think that should be fine. One thing I'm considering is whether or not to switch to an s-expr representation like Wasm, to make it easier to maintain and extend, and to keep the parser dead simple - but until that happens, I think this is likely to be the only unambiguous way to represent components in the textual format.

Plus, I'd like to print Component imports as well. So far I drafted them like

I think we should use attributes for metadata items like the "call" annotation (which I think should specify a calling convention, rather than something one off, since call and exec will have different calling conventions, and call will have only one possible calling convention), as well as the MAST root. Otherwise I think that looks fine. We may have to support quoted identifiers so that those used for component imports/exports are more unambiguous, but I'll have to confirm whether or not it is actually an issue.

@greenhat
Copy link
Contributor Author

I'm thinking of putting module's body in curly braces (+indent) and printing them sequentially

I think that should be fine. One thing I'm considering is whether or not to switch to an s-expr representation like Wasm, to make it easier to maintain and extend, and to keep the parser dead simple - but until that happens, I think this is likely to be the only unambiguous way to represent components in the textual format.

I got back to this PR to add curly braces, and I don't know why I have not replied to this earlier, but I like the s-expr idea very much. Admittedly, after staring into WAT for so long, I find it very readable.

@greenhat greenhat marked this pull request as ready for review February 29, 2024 13:22
@greenhat greenhat requested a review from bitwalker February 29, 2024 13:23
@greenhat greenhat force-pushed the greenhat/wasmcm-itest-support branch from fe0cbde to 2d39974 Compare February 29, 2024 14:10
@greenhat greenhat changed the title Add Wasm component translation support to the integration tests [1/x] Add Wasm component translation support to the integration tests Feb 29, 2024
@bitwalker
Copy link
Contributor

@greenhat I think we'll want to reimplement these changes on #149 since we'll need to port the changes over anyway. Since the PR updates all of the tests, all that is really needed are the new IR components, implementing the formatting using PrettyPrint rather than Display, and adding the new grammar rule for components. If you like, I can do that for you, since I can probably do the refactoring faster due to familiarity, but I'll leave it up to you.

In any case, PR looks good otherwise, so if you are cool with me refactoring it for you, I'll go ahead and do that

@greenhat
Copy link
Contributor Author

@greenhat I think we'll want to reimplement these changes on #149 since we'll need to port the changes over anyway. Since the PR updates all of the tests, all that is really needed are the new IR components, implementing the formatting using PrettyPrint rather than Display, and adding the new grammar rule for components. If you like, I can do that for you, since I can probably do the refactoring faster due to familiarity, but I'll leave it up to you.

In any case, PR looks good otherwise, so if you are cool with me refactoring it for you, I'll go ahead and do that

Sure. I appreciate it if you'd take care of that. Thanks.

components and Miden SDK tests.

The panic handler and global allocator is now set in the `lib.rs` file
of each component crate. Before, the `cargo-component-bindings` was not
built correctly for `no_std` and was linking the stdlib allocator.

The Rust bindings for the WIT are now generated in the `src` directory
of the component crate, instead of the `target/.../bindings` directory.

The Wasm `bulk-memory` proposal is enabled via `RUSTFLAGS` so that
`memcpy` function import is replaced with Wasm `memory.copy` op.
@bitwalker bitwalker force-pushed the greenhat/wasmcm-itest-support branch from 2d39974 to 1960cfd Compare March 15, 2024 04:19
@bitwalker bitwalker merged commit 0be6a64 into main Mar 15, 2024
@bitwalker bitwalker deleted the greenhat/wasmcm-itest-support branch March 15, 2024 04:20
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