-
-
Notifications
You must be signed in to change notification settings - Fork 320
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
WIP New Dev Backend Representation #7397
base: main
Are you sure you want to change the base?
Conversation
Got the basic ROAR structures to pretty print. Also, the tests currently in `lib.rs` will be moved somewhere more suitable once i've got the rest of it working
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.
Yeah, this seems like it should be a solid basis overall. I always think way too much about exact layout and DOD, but far more important than that is getting something working and maintainable. Left a handful of comments.
The ones around arena allocation and at least asserting the sizes of types are probably the most useful to start with early. They will be the hardest to fix up later.
crates/compiler/roar/src/ops.rs
Outdated
///Copy bits from a regular register to a floating point register, without conversion | ||
ToFloatRaw, | ||
///Shift left instruction, whether arithmetic or logical depends on `Sign` flag | ||
ShiftLeft(Sign,ByteSize), |
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 double check if rust flattens nested enums like this (maybe there is a way to force it to do so). I don't think so, which ends up getting annoying for space efficiency. We would like the opcode to take up a single byte, but I think this will be 3 bytes. That said, if everything else ends up aligning to 8 bytes and we don't do SOA form, the padding would allow for this.
Anyway, exact size can be optimized later, but I think we will end up wanting size asserts on all of these types just to make sure that don't accidentally get bloated.
you can see some examples in mono ir as:
roc_error_macros::assert_sizeof_wasm!(CallType, 36);
roc_error_macros::assert_sizeof_non_wasm!(Literal, 3 * 8);
crates/compiler/roar/src/ops.rs
Outdated
pub struct Operation { | ||
pub output : Output, | ||
pub opcode : OpCode, | ||
pub inputs : Vec<Input> |
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.
How many inputs are possible? If just two or three, I would inline them directly and use the nul register for unused inputs.
If it is only 1 or 2 ops are special (like call), it may be worth storing it's arguments sideband in a separate list. The input here would just be an index to that separate list.
If many ops are special, or just for simplicity, we can use a dynamic length container here, but we should use an arena allocator. I think the type we would want would be:
pub inputs: &'a [Input],
And we would be using a bumpalo arena to allocate them. In our code, you will regularly see .into_bump_slice
and env.arena.alloc(...)
.
crates/compiler/roar/src/proc.rs
Outdated
#[non_exhaustive] | ||
pub struct Proc { | ||
///The instructions themselves | ||
pub instructions : Vec<Operation>, |
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.
These should be a bump slice if they will stay one length after allocation. If they can change length, probably should be a bumpalo::Vec. Preferably though we avoid changing length if possible. Ditto with inputs below (though inputs is almost certainly just a slice)
crates/compiler/roar/src/proc.rs
Outdated
///All the registers it uses | ||
uses : Option<Vec<Output>>, | ||
///It's name in the AST | ||
symbol : Option<symbol::Symbol>, |
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 don't really follow these and when they would or would not be used (like a proc always has a symbol, right?), but I assume you have a goal for them.
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.
For symbol, it is for the purpose of debugging (if the program has a runtime error), and as for the uses
, it is a list of registers modified by the procedure, which is chached (for a given function) after it's first computation
crates/compiler/roar/src/storage.rs
Outdated
Signed(i64), | ||
Unsigned(u64), | ||
Float(f64), | ||
Char(char) |
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 am not 100% sure your plan for literals, but a few notes:
- we have integers upto 128 bit
- float64 is layed out different than float32, which may be important in some case.
- We have dec as the default fractional type which you can think of as an i128.
- char doesn't exist in roc. something like
'a'
can be any integer type.
Not sure if this is better or worse now... Not done with comments, will continue with them tommorow
The sense that it is working is that it Is barely started Crashes the compiler Uses poor style And prevents roc from working, even when it isn't used This will be (hopefully) temporary, and an actual solution found at some point
As discussed in many places, Roc's current dev backend is quite slow and fragile. ROAR is meant to fix these problems, by being quicker and cleaner than the current psuedo-assembly, while still being integrated into the existing generation process, ultimately only having to re-implement part of the dev backend as a result