-
Notifications
You must be signed in to change notification settings - Fork 160
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
Implement kernel compilation in the assembler #1418
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Left a few suggestions
@@ -5,6 +5,9 @@ use crate::{ | |||
use alloc::vec::Vec; | |||
use miden_crypto::hash::rpo::RpoDigest; | |||
|
|||
// KERNEL |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Isn't it cleaner to only have sections when there are more than 1 definitions in a file?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had the same thought, kinda just feels like clutter when there is nothing else in the file except the item in question.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I usually put these in to separate the "imports" section from the main code. It is a kind of a visual clue for "important code starts after this line".
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, looks good! I left a few comments where I think we should consider some small changes, hence why I'm marking this as requesting changes, but if there's pushback on those, let me know. Once I've caught up again later today, I'll re-review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left a suggestion, which we can implement in a subsequent PR if we decide to go down that route
kernel: Kernel, | ||
kernel_info: ModuleInfo, | ||
library: CompiledLibrary, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was thinking a KernelLibrary
would only wrap a CompiledLibrary
(and would make all its checks in the constructor). Then the ModuleInfo
and Kernel
can be derived from the CompiledLibrary
in methods. The advantage of that approach would be that the struct would be simpler to understand, since it wouldn't store any duplicate information.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how I had it originally but then noticed that the constructor and into_parts()
method were almost identical (e.g., we need to build a Kernel
at construction time to validate that it can be built without errors). So, given that additional memory footprint is likely to be minimal (kernels can have at most 256 procedures), decided not to throw out all the stuff we built in the constructor and save it with the struct.
I do agree that we could probably refactor things a bit here (e.g., maybe Kernel
should be just a newtype over ModuleInfo
) but I'm leaving this to future PRs.
This PR builds on #1401 and adds ability to assemble kernel libraries. Specifically:
Assembler::assemble_kernel()
method which takes a single module and returns a compiled library. We can extend this in the future to take a list of modules one of which will be the kernel module.Assembler::with_kernel()
constructor which takes a compiled library representing the kernel to be used by the assembler.ModuleGraph::with_kernel()
constructor and the new error typeAssemblerError
.Also, as a part of this PR I removed a bunch of error variants from
AssemblyError
which seemed to be unused.Not done in this PR: