Skip to content

[1/x] Miden SDK as plain Rust dependencies #140

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 28 commits into from
Apr 10, 2024
Merged

Conversation

greenhat
Copy link
Contributor

@greenhat greenhat commented Mar 5, 2024

Ref #138
Ref #118

This PR adds:

  • miden-sdk crate with a small subset of tx kernel API functions, one for each transformation strategy, with partial transformation of the parameters and results on the Rust side (bindings);
  • infra in the frontend-wasm crate to recognize and transform parameters and results for function calls for these well-known functions during the Wasm translation;

The native Felt type is not implemented in this PR and will be added in the next PR.

@greenhat greenhat linked an issue Mar 5, 2024 that may be closed by this pull request
@greenhat greenhat changed the title Miden SDK as plain Rust dependencies [5/x] Miden SDK as plain Rust dependencies Mar 5, 2024
@greenhat
Copy link
Contributor Author

greenhat commented Mar 6, 2024

@bitwalker I got to the point where I need to figure out where to put the Wasm module import (Miden SDK extern function) in the IR.

How to represent a Miden SDK extern function Wasm import in IR?

Component-level import vs. module-level import.

As a component-level import

Pros (vs. module-level import):

  • Concentrating lowering/lifting logic in one place; One can view the generation of the adapter function from C ABI to the kernel tx ad-hoc ABI as a lowering/lifting of sorts.
  • When we switch to WIT-based Miden SDK in the future, the lowering/lifting logic's place will remain unchanged, only the logic itself will change.

Cons (vs. module-level import):

  • Introduces a different kind of component import in the IR that will require special handling;
  • Wasm component import is called, while Miden SDK import is execed; Although one could argue that it's a lowering/lifting implementation detail.
  • Requires generating a synthetic component to wrap the Wasm module since the Rust compilation produces only a Wasm module; Although, we might need to do this anyway for tables (see comment) so this point might actually be moved to pros.

I want to go with component-level import, and I'm starting to draft the code for it. What do you think?

greenhat added a commit that referenced this pull request Mar 6, 2024
See considerations on component vs. module import in #140 (comment)
@greenhat greenhat linked an issue Mar 8, 2024 that may be closed by this pull request
@greenhat greenhat force-pushed the greenhat/i138-rust-miden-sdk branch 2 times, most recently from 00612b1 to 48622fa Compare March 14, 2024 13:41
@bitwalker bitwalker force-pushed the greenhat/i110-p2id-note-translation branch from 4815b41 to 87164d6 Compare March 15, 2024 07:43
Base automatically changed from greenhat/i110-p2id-note-translation to main March 15, 2024 07:44
@bitwalker
Copy link
Contributor

@greenhat Now that I've finished rebasing/merging the other PRs, since you are still working on this one, you'll need to create a new temporary branch off of main, then cherry-pick your commits made for this PR to that new branch (not the other commits that GitHub thinks are now part of this PR, so a simple git rebase main isn't an option here), then force push, i.e.

  1. git checkout main && git pull origin main && git checkout -b temp
  2. Cherry-pick c564e5b and everything after it to temp
  3. While still in temp branch: git branch -D greenhat/i138-rust-miden-sdk && git checkout -b greenhat/i138-rust-miden-sdk && git push --force origin greenhat/i138-rust-miden-sdk && git branch -D temp.

You probably already know the process, but just to make sure everything is painless as possible, that's what I would do. There will be conflicts due to the formatter changes, but most things should be straightforward.

As mentioned in one of my review comments on the other PRs: the Tuple type was discarded in favor of Struct types, since there is no semantic difference between them, so if you run into any conflicts due to that, just replace any uses of Tuple with Struct and you should be good to go.

Happy to answer any questions about any of the new changes as well, just let me know.

@greenhat
Copy link
Contributor Author

Sure, will do. Thanks for the detailed steps and thanks for taking care of my previous PRs.

@greenhat greenhat force-pushed the greenhat/i138-rust-miden-sdk branch from 84b4423 to 1356d63 Compare March 15, 2024 14:52
Miden SDK functions imports in the HIR textual form.
@greenhat greenhat force-pushed the greenhat/i138-rust-miden-sdk branch from 87d8060 to 82842eb Compare March 22, 2024 15:22
@greenhat greenhat requested a review from bitwalker March 27, 2024 16:15
@greenhat
Copy link
Contributor Author

@bitwalker Thank you for the detailed review notes! I addressed them all. Please, make another round.


/// No transformation needed
#[inline(always)]
pub fn no_transform(
Copy link
Contributor

Choose a reason for hiding this comment

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

Just as a heads up, we should make sure that any function type which would be handled as no_transform, uses Abi::Canonical so that it goes through the normal lowering process. For the other Abi variants, we either want to transform them such that they are Canonical, or that they require no further transformation aside from direct translation to Miden Assembly.

To be clear, haven't checked to see if that's already the case anyway, but wanted to raise it just in case that conflicts with what your operating assumptions are.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! Actually, since all the Miden ABI transformation happens in the frontend, I'll use Abi::Canonical for all the function type definitions because all of them should have this ABI after the processing regardless whether an actual transformation was conducted. This means that Abi::Miden is not used, so it can be deleted. Keep in mind that Abi is not included in Signature. Besides that, I removed (abi XXXXX) in ComponentImport textual form and printed it as part of FunctionType in the textual form.
All the aforementioned changes are made in the #156 due to their invasive nature to the two subsequent PRs that makes rebasing a PITA. See commit 82870e3

Copy link
Contributor

@bitwalker bitwalker left a comment

Choose a reason for hiding this comment

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

Looks good! Just a couple last questions/notes, but I'll sign off and let you respond/handle those as you see fit.

I'll try to get to your next PR tomorrow, Friday at the latest!

@greenhat greenhat force-pushed the greenhat/i138-rust-miden-sdk branch from 7f19cfa to a5134b5 Compare March 28, 2024 14:59
@greenhat greenhat requested a review from bitwalker March 28, 2024 15:00
@bitwalker
Copy link
Contributor

LGTM!

@greenhat greenhat merged commit f54921b into main Apr 10, 2024
@greenhat greenhat deleted the greenhat/i138-rust-miden-sdk branch April 10, 2024 15:08
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.

Native Miden VM Felt, Word types and ops Miden SDK as a plain Rust dependencies
2 participants