-
Notifications
You must be signed in to change notification settings - Fork 55
Fix HIR and MASM handling in --emit option, add internal group and folder path support
#836
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
Conversation
7651d0b to
f293423
Compare
bitwalker
left a comment
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! I have a few minor things I'd like to tweak, but overall I think this should do nicely 👍
| /// working directory. When set to a non-empty value other than `1`, it is treated as the output | ||
| /// directory. | ||
| fn maybe_dump_cargo_expand(test: &CargoTest, rustflags_env: Option<&str>) { | ||
| let Some(value) = std::env::var_os("MIDENC_EXPAND") else { |
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 know it's more verbose, but could we use MIDENC_EMIT_MACRO_EXPAND for this? For a couple of reasons:
- It keeps our environment variables a bit more structured, and eases future additions.
- It is much clearer what the purpose of it is, and it's relationship to
--emit - LLMs can probably better understand it. Which isn't a priority for me, but doesn't hurt.
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.
Sure. Done.
midenc-session/src/outputs.rs
Outdated
|
|
||
| /// Returns the subset of [OutputType] values considered "intermediate" for convenience | ||
| /// emission (WAT, HIR, MASM). | ||
| pub fn inter() -> [OutputType; 3] { |
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.
Nit: I'd prefer just ir for this (since all of these are intermediate representations, it's better shorthand, and easier to remember than "inter" IMO).
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.
Done.
Funnily enough, initially I thought about naming it ir but then I decided to go with inter to emphasize that this is a group of multiple intermediate formats and not the HIR representation.
midenc-session/src/outputs.rs
Outdated
|
|
||
| /// Returns the subset of [OutputType] values considered "intermediate" for convenience | ||
| /// emission (WAT, HIR, MASM). | ||
| pub fn inter() -> [OutputType; 3] { |
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'd make the return type &'static [OutputType] here, and mark it const, that way we don't have to hardcode the size of the array in the signature, and it's just as useful at compile-time as an array.
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.
Done
midenc-session/src/outputs.rs
Outdated
| /// Emit a common set of intermediate artifacts (WAT, HIR, MASM) into a directory. | ||
| /// | ||
| /// The directory path is interpreted relative to the session working directory. | ||
| Inter { |
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'd suggest we name this variant Subset, and definite it like so:
enum OutputTypeSpec {
// ...
Subset {
output_types: SmallVec<[OutputType; 3]>,
path: Option<OutputFile>,
},
// ...
}This makes OutputTypeSpec a bit more versatile, and doesn't bake into it the assumption that emitting multiple outputs has to be just intermediate representations, but any combination of two or more output types.
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.
Good idea! Done.
midenc-session/src/outputs.rs
Outdated
| PossibleValue::new("mast").help("Merkelized Abstract Syntax Tree (text)"), | ||
| PossibleValue::new("masl").help("Merkelized Abstract Syntax Tree (binary)"), | ||
| PossibleValue::new("masp").help("Miden Assembly Package Format (binary)"), | ||
| PossibleValue::new("inter").help("WAT + HIR + MASM (text, optional directory)"), |
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.
Per my other comment, let's name this value ir instead of inter
midenc-compile/src/compiler.rs
Outdated
| if output_types.is_empty() { | ||
| let has_final_output = output_types | ||
| .keys() | ||
| .any(|ty| matches!(ty, OutputType::Mast | OutputType::Masl | OutputType::Masp)); |
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.
We should remove the Masl output type - it is no longer useful, and I'm in the process of removing Library entirely from the assembler
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.
Done.
midenc-compile/src/stages/parse.rs
Outdated
|
|
||
| use super::*; | ||
|
|
||
| /// A wrapper that emits WebAssembly text format (WAT). |
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.
Nit: Probably better to define this in the midenc-frontend-wasm crate, and it'll reduce the clutter in this file as an extra benefit.
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.
Done.
midenc-compile/src/stages/codegen.rs
Outdated
| /// | ||
| /// The format matches the [core::fmt::Display] implementation for [MasmComponent], which is | ||
| /// defined in `codegen/masm/src/artifact.rs`. | ||
| struct MasmComponentEmit<'a>(&'a MasmComponent); |
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.
Similarly to WasmEmit, let's move this to the midenc-codegen-masm crate.
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.
Done.
9549c10 to
af7f2f0
Compare
|
Rebased and ready for another round. |
package output location.
add Debugging section in the README.
af7f2f0 to
844e236
Compare
|
Rebased again after #810 was merged. Ready for merge. |
Close #724
Close #816
This PR:
--emit=hirandmasmand preserves Miden package output location;--emit=wat[=<path>]option to emit wat files;--emit=inter[=<path>]option to emit wat, hir, masm files;cargo miden buildoptions to themidenc(instead of only--emit);MIDENC_EXPAND=[<path>]env var handling in the compiler test suite to emit Rust code with expanded macros;