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

Plafer mast library compilation #1401

Merged
merged 188 commits into from
Jul 29, 2024
Merged

Plafer mast library compilation #1401

merged 188 commits into from
Jul 29, 2024

Conversation

plafer
Copy link
Contributor

@plafer plafer commented Jul 16, 2024

Implements follow-up to #1370, as detailed in #1226 (comment).

assembly/src/tests.rs Outdated Show resolved Hide resolved
Comment on lines 325 to 326
match pending_module {
PendingModuleWrapper::Ast(pending_module) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If compiled modules are added to the module graph "up-font" (i.e., at construction time), the probably won't make it into the pending module set and may not need to have special handling for compiled modules here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think removing the pending set is intricately linked with #1416. The main issue right now is with module rewriting here, which is the only reason why we need pending modules to be Box<Module> instead of Arc<Module> (i.e. we need ownership to be able to mutate it). I am also assuming that using Arc<Module> in our public API is a requirement.

But all this module rewriting goes away if we only allow modules to be added all at once, and the graph to be "recomputed" only once (which can be done with #1416). And then removing the pending module list becomes easy.

Copy link
Contributor

@bobbinth bobbinth Jul 26, 2024

Choose a reason for hiding this comment

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

I think incremental additions of AST modules is something we may want to keep in the public API, but since we generate the output only once, we probably can do the "recompute" part only once as well. So, basically:

  • All compiled modules are added at construction time.
  • AST modules can be added one-by-one, but nothing is recomputed at this point.
  • When the user calls assemble_program(), assemble_library() etc. the graph is computed and this happens only once.

And yes, I agree this is something to be done in/after #1416.

Copy link
Contributor

@bobbinth bobbinth 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! Thank you!

Comment on lines +530 to +531
// Note: For compiled procedures, we can't check further if they're compatible,
// so we assume they are.
Copy link
Contributor

Choose a reason for hiding this comment

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

Note for the future: if two procedures have the same MAST root, they would always have the same number of locals (because changing the number of locals would change the MAST root). So, if the number of locals differs somehow between two procedures with the same MAST root, one (or both) of them have incorrect number of locals in the metadata.

@plafer plafer merged commit 78b55a9 into next Jul 29, 2024
9 checks passed
@plafer plafer deleted the plafer-mast-library-compilation branch July 29, 2024 18:32
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.

4 participants