feat/refactor: general improvements to ir, workspace, and implementation of new analyses/transforms

[bitwalker]

This is a big set of changes containing much of my work from the last month or so that I would like to get merged today/tomorrow. See the individual commits for information on the changes contained within.

This is going to get merged pretty much as-is, and iterated on further in more targeted PRs later. That said, feel free to use this opportunity to leave comments, ask questions, etc.

NOTE: There are still a few minor details that are still work-in-progress. Namely the Program and Linker structs, which go together, and are largely stubbed out at the moment until I finish up work on the codegen crate, which is not present in this changeset. That will follow in its own PR in the next couple days.

@greenhat This likely affects you in a couple (relatively minor) ways - I'm most interested in your feedback on whether there are things you'd like to see, or that you find awkward. Some of the public interfaces, particularly around the builders are not super refined yet, so this is definitely the time to tweak how those work.

@jjcnn There are a few small changes to how the IR gets output (we have additional data that needs to be represented now), let me know if you have any questions about that when it comes to the parser for the IR. Some of the changes should have actually made it easier for constructing the IR in the parser, since there is basically no dependency between a Function and its parent Module anymore. The main thing to be aware of is the new Ident, FunctionIdent, and Symbol types, used for identifiers/interned strings.

[jjcnn]

I would like to get merged today/tomorrow.

Sorry, but this isn't going to happen. The PR contains around 10,000 lines of code, and there is just no way this gets reviewed properly in a day (especially not a day when github has issues).

You can have a proper review in the time it takes to do a proper review, or you can spend some time separating the PR into multiple, smaller PRs (which is Best Practice).

Also, do you mind describing the actual changes this PR makes, so that I have a proper chance of figuring out what is going on in the code? It is not particularly helpful to refer to it as "a big set of changes containing much of my work from the last month or so" - not only is that not helpful to the reviewer, but we also need the description in the commit history so that we can track the reasons why changes were made.

[bitwalker]

Sorry, but [review by today/tomorrow] isn't going to happen

That's fine, we should merge this anyway. I was quite clear that these changes are meant to be merged without an expectation of in-depth review, and that any issues raised here, whether before or after the changes are merged, would be addressed in subsequent bug reports/PRs. The entire point was to get us back in a cycle of small changes, ASAP.

You can have a proper review in the time it takes to do a proper review, or you can spend some time separating the PR into multiple, smaller PRs (which is Best Practice).

I have no intention of breaking up this PR into smaller ones - I'm fine with these changes as-is, at least at this point in time. To the extent that there are issues with the code here, I expect those to be raised as bugs; addressed with subsequent PRs that introduce missing or stubbed-out functionality (e.g. the linker); or are addressed in the course of making more general code quality improvements, after things stabilize and we have time to polish. Whether you wish to do a proper review is your prerogative, but that was not the purpose of this PR. I would prefer that you sign off, and review at your leisure, so that @greenhat and I can start working on merging our streams of work together as soon as possible - but if you do not want to do that, then another alternative to consider is pairing up and walking through the changes together to try and simplifiy the process, since having me on hand to explain and provide context will save a lot of turnaround time, and likely be more useful to you anyway. Aside from that, I think you'll need to justify your reticence to merge this PR, aside from questions of process, if you expect that your review will push beyond this week to complete. If need be, I'll merge these changes directly, but I'm only inclined to do that if it starts blocking other work that is in progress. For the time being, I appreciate any review you have time to provide.

Also, do you mind describing the actual changes this PR makes, so that I have a proper chance of figuring out what is going on in the code?

You'll have to be more specific - the individual commit messages are descriptive, in some cases quite verbose, and the code itself is well documented, especially the analyses and transforms. I also pointed out in my initial PR description the specific areas relevant to what you are working on that you may want to look at. What areas of the code in particular are you finding unclear and could be improved with additional documentation?

In an effort to save time and perhaps anticipate some contextual questions you may have, here is some additional (perhaps redundant) summary:

• To the extent that there is a notion of a complete feature(s) here, it is in the implementation of the analyses and transforms that operate on the IR, which are well documented. If there are critical docs missing, let me know and I'll make sure to add them. The transforms in particular are pre-requisites for the stackification pass, which will come in a follow-on PR.
• Everything else is either something required by the Wasm frontend, or falls out from working out the kinks in the IR that was initially sketched out; mostly as a result of working through lowering the full instruction set to Miden Assembly.
• Support for globals is required by Wasm (and many languages more generally). Those in turn require us to introduce a linking step, in order to de-duplicate and lay out globals on the heap prior to code generation, since we need to translate access of global symbols into absolute addresses. There is no native notion of globals in Miden, so we are implementing it in software for now.
• Most changes to Module and Function are intended to make it easier to parellelize the compiler, allowing one to compile using a function per-thread, or a module per-thread, or a combination of the two, while making it easier to link them all together later.
• There are changes to Signature to better represent certain low-level aspects of arguments and results, describe linkage, and to make the signature itself independent of function name
• The Visibility bitflags were removed in favor of the Linkage enum, which is used for both functions and globals.
• ExternalFunction was introduced to represent a dependency on another function, from within a Function. It consists of just the name and signature, and corresponds to a function declaration, rather than definition.
• Function is now always created with an entry block that matches the function signature, as it was never necessary to represent bodyless functions anyway.
• There is no purpose in explaining each of the instruction changes, because the initial instruction set was a rough draft to begin with - with a number of missing operations, insufficient specificity, incorrect typing rules, etc. However, some of these changes are mentioned in the relevant commit message. Most of them are obvious, but if you have specific changes you are curious about, I'm happy to provide more details.
• The introduction of the ModuleBuilder is intended to make imperatively building a module and it's functions more concise, as well as provide additional validation when doing so. This is primarily for the benefit of constructing IR for testing, but obviously it is useful more generally as well. FunctionBuilder still exists, and can be used either to build up a function from scratch, or modify an already constructed function.
• A significant number of asserts/validations were added to the InstBuilderBase trait, to protect against the construction of invalid IR. Some supporting macros were added to make expressing common validations easier.
• Symbol/string interning was added to make it possible to use natural identifiers to reference modules/functions efficiently, rather than arbitrary integer ids, while retaining the ability to get the string representation, order by name, etc. All uses of String for identifiers was replaced with an appropriate interned equivalent. This also was important for decoupling Module and Function for multi-threaded compilation.
• Our Rust toolchain was out of date, that was updated to the latest stable release

If I've left something out, feel free to ask questions.

[greenhat]

@greenhat This likely affects you in a couple (relatively minor) ways - I'm most interested in your feedback on whether there are things you'd like to see, or that you find awkward. Some of the public interfaces, particularly around the builders are not super refined yet, so this is definitely the time to tweak how those work.

I rebased my branch on this one and I'm going through the process of exploring your changes and adopting my code. I expect to finish it today (your morning) and provide feedback.

[bitwalker]

I rebased my branch on this one and I'm going through the process of exploring your changes and adopting my code. I expect to finish it today (your morning) and provide feedback.

Awesome! Let me know if you have any questions, identify any bugs, or find anything missing/awkward to work with. We can address those changes/improvements in subsequent PRs, but I'll make sure to prioritize anything that is a blocker.

[greenhat]

I rebased my branch on this one and I'm going through the process of exploring your changes and adopting my code. I expect to finish it today (your morning) and provide feedback.

Awesome! Let me know if you have any questions, identify any bugs, or find anything missing/awkward to work with. We can address those changes/improvements in subsequent PRs, but I'll make sure to prioritize anything that is a blocker.

I adopted my code and fixed the build and tests (except the test for unsupported ops). I pushed my branch in my PR #17
I had to make two changes to your code (see comments in my PR):

• Get FunctionBuilder from ModuleFunctionBuilder Wasm -> Miden IR translation #17 (review)
• Fix type mismatch for Load Miden IR op Wasm -> Miden IR translation #17 (review)

Also, I see binary and comparison ops expect only integers now. Makes total sense. Following our call, I think it's better to remove all the float ops translations and return the unsupported error. Sound good?

Besides that, I did not find anything missing or awkward. I have not tried to implement globals and data segments yet. That would be my next move after I fix some minor todos I left in the code on rebase.

[bitwalker]

Following our call, I think it's better to remove all the float ops translations and return the unsupported error. Sound good?

Yep, that sounds good for now, we'll probably need to add dedicated floating point operations anyway when it comes time to support that in the future, if we ever decide to do so.

As an aside, you probably noticed, but just in case it got lost in the sea of changes, I did add support for a few of the unsupported ops you had listed, namely unreachable and select. I can't recall if there were any others. I need to dig into the semantics of some of the Wasm instructions in the unsupported list to determine whether or not they can be handled with the instructions that are there now, or if we need new ones, but I don't know that any of them are particularly critical to implement at this point, but let me know if you think there are some big ones that would be nice to have.

Besides that, I did not find anything missing or awkward. I have not tried to implement globals and data segments yet. That would be my next move after I fix some minor todos I left in the code on rebase.

We should probably discuss data segments before you dig in to the implementation there, shoot me a message when you have time, and we can set up a quick call to work through it quick, make sure we have what we need, and that it will play nice with how things are going to be represented in MASM.

As for global variables, I think everything you need should be there now, but there may be some kinks to work out, or improvements to the ergonomics of the APIs we can make. I should note that a Function has no notion of the actual global variable data itself, instead it has a notion of a global value, like in Cranelift, and global variables are actually defined/exported at the Module level. The linker, which isn't fully implemented yet, will be responsible for stitching together the global data, validating that all symbols are present, de-duplicating, that sort of thing. Then, during code generation, we'll compute absolute addresses to the global data based on the layout chosen by the linker. One of the things you can assume is that all global variables will be laid out in memory so that their addresses are either word- or felt-aligned, so that we can always use aligned loads/stores with them. We may even be able to use native pointer types with them, but that's an optimization we can look into later.

[greenhat]

As an aside, you probably noticed, but just in case it got lost in the sea of changes, I did add support for a few of the unsupported ops you had listed, namely unreachable and select. I can't recall if there were any others. I need to dig into the semantics of some of the Wasm instructions in the unsupported list to determine whether or not they can be handled with the instructions that are there now, or if we need new ones, but I don't know that any of them are particularly critical to implement at this point, but let me know if you think there are some big ones that would be nice to have.

Thanks for reminding me about unreachable and select! I added them to my PR's todo list. I've scanned the unsupported list, and the following ops caught my eye:

• signed integer comparison ops: gt_s, le_s, etc.
• i32.wrap_i64 (i64_val mod 2^32 should work, so probably no need for separate IR op);
• ctz, clz;
• shr_s (bitwise shift right signed).

They seem like a "nice to have" ops.

We should probably discuss data segments before you dig in to the implementation there, shoot me a message when you have time, and we can set up a quick call to work through it quick, make sure we have what we need, and that it will play nice with how things are going to be represented in MASM.

Sure! Let me clean up my todo list (new ops, globals, etc.), and I'll ping you when I get close to it.

As for global variables, I think everything you need should be there now, but there may be some kinks to work out, or improvements to the ergonomics of the APIs we can make. I should note that a Function has no notion of the actual global variable data itself, instead it has a notion of a global value, like in Cranelift, and global variables are actually defined/exported at the Module level. The linker, which isn't fully implemented yet, will be responsible for stitching together the global data, validating that all symbols are present, de-duplicating, that sort of thing. Then, during code generation, we'll compute absolute addresses to the global data based on the layout chosen by the linker. One of the things you can assume is that all global variables will be laid out in memory so that their addresses are either word- or felt-aligned, so that we can always use aligned loads/stores with them. We may even be able to use native pointer types with them, but that's an optimization we can look into later.

Sounds good! I'll dig into it.

[bitwalker]

I've scanned the unsupported list, and the following ops caught my eye:
..
signed integer comparison ops: gt_s, le_s, etc.

I've pushed a commit that expands our type system with signed and unsigned equivalents, all of the standard operators (comparison, arithmetic, etc.) will be compiled based on the semantics of the types involved. I still have to work out what to do about signed integers in general - our only native option in MASM for signed integers are field elements, the support for u32 and u64 is only useful for unsigned ops, as you'd expect. That probably means that we either need to implement our own primitives for representing, say, signed 32-bit integers, or promote all signed 32-bit integers to field elements, but that comes with its own complexities. I suspect for now we'll need some combination of the two, i.e. using field elements for signed integers, with some additional generated code around ops involving them to protect the range of the type in question. I think we can probably kick this can down the road a bit though.

i32.wrap_i64 (i64_val mod 2^32 should work, so probably no need for separate IR op);

I think from your end, you can use trunc for this, since I think the semantics are basically discarding the upper 32-bits; but I'm just throwing that out there off-hand, it may turn out we need a dedicated opcode for this.

ctz, clz

I've got these on my todo list, because I think I'll need them for some of the low-level stuff I'm doing in the address space translation layer, but not sure exactly when I'll get them implemented. I'll keep you posted.

shr_s (bitwise shift right signed)

I'm assuming this is a standard arithmetic shift (i.e. sign-extending the high bits after shifting). There isn't native support for this in MIden, but this is something we can emit an instruction sequence for pretty easily, just like we will for ctz/clz and others. I think for now we can leave this unsupported, but adding support for it should be straightforward when we have a bit of time for it.

If you see any instructions coming through from Rust-compiled Wasm modules that are blocking simple programs, can you leave a comment on #19? We can track what is unsupported, but wanted there.

[greenhat]

I've scanned the unsupported list, and the following ops caught my eye:
..
signed integer comparison ops: gt_s, le_s, etc.

I've pushed a commit that expands our type system with signed and unsigned equivalents, all of the standard operators (comparison, arithmetic, etc.) will be compiled based on the semantics of the types involved. I still have to work out what to do about signed integers in general - our only native option in MASM for signed integers are field elements, the support for u32 and u64 is only useful for unsigned ops, as you'd expect. That probably means that we either need to implement our own primitives for representing, say, signed 32-bit integers, or promote all signed 32-bit integers to field elements, but that comes with its own complexities. I suspect for now we'll need some combination of the two, i.e. using field elements for signed integers, with some additional generated code around ops involving them to protect the range of the type in question. I think we can probably kick this can down the road a bit though.

Nice! I'll rebase and try them out.

i32.wrap_i64 (i64_val mod 2^32 should work, so probably no need for separate IR op);

I think from your end, you can use trunc for this, since I think the semantics are basically discarding the upper 32-bits; but I'm just throwing that out there off-hand, it may turn out we need a dedicated opcode for this.

Yeah, trunc should do the job.

shr_s (bitwise shift right signed)

I'm assuming this is a standard arithmetic shift (i.e. sign-extending the high bits after shifting). There isn't native support for this in MIden, but this is something we can emit an instruction sequence for pretty easily, just like we will for ctz/clz and others. I think for now we can leave this unsupported, but adding support for it should be straightforward when we have a bit of time for it.

Yep, here's the spec for it:
https://webassembly.github.io/JS-BigInt-integration/core/exec/numerics.html#op-ishr-s

If you see any instructions coming through from Rust-compiled Wasm modules that are blocking simple programs, can you leave a comment on #19? We can track what is unsupported, but wanted there.

Sure. Will do.

[greenhat]

There is also ValueData::span(), which now seems redundant.

[bitwalker]

Yeah, since it only provides the span for a single variant, it probably can go away

[jjcnn]

Approved, as agreed in today's meeting.