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

WIP New Dev Backend Representation #7397

Draft
wants to merge 11 commits into
base: main
Choose a base branch
from
Draft

Conversation

wizard7377
Copy link
Contributor

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

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
Copy link
Collaborator

@bhansconnect bhansconnect left a 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.

Cargo.toml Outdated Show resolved Hide resolved
crates/compiler/roar/src/display.rs Outdated Show resolved Hide resolved
///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),
Copy link
Collaborator

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);

pub struct Operation {
pub output : Output,
pub opcode : OpCode,
pub inputs : Vec<Input>
Copy link
Collaborator

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(...).

#[non_exhaustive]
pub struct Proc {
///The instructions themselves
pub instructions : Vec<Operation>,
Copy link
Collaborator

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)

Comment on lines 18 to 21
///All the registers it uses
uses : Option<Vec<Output>>,
///It's name in the AST
symbol : Option<symbol::Symbol>,
Copy link
Collaborator

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.

Copy link
Contributor Author

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/proc.rs Outdated Show resolved Hide resolved
crates/compiler/roar/src/storage.rs Outdated Show resolved Hide resolved
Comment on lines 20 to 23
Signed(i64),
Unsigned(u64),
Float(f64),
Char(char)
Copy link
Collaborator

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:

  1. we have integers upto 128 bit
  2. float64 is layed out different than float32, which may be important in some case.
  3. We have dec as the default fractional type which you can think of as an i128.
  4. 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
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.

2 participants