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

Implement new parsing pipeline #490

Merged
merged 49 commits into from
Nov 21, 2022
Merged

Implement new parsing pipeline #490

merged 49 commits into from
Nov 21, 2022

Conversation

vlopes11
Copy link
Contributor

@vlopes11 vlopes11 commented Nov 9, 2022

WiP

@vlopes11 vlopes11 marked this pull request as draft November 9, 2022 23:00
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I left some comments inline.

assembly/src/lib.rs Outdated Show resolved Hide resolved
assembly/src/parsers/ast/mod.rs Outdated Show resolved Hide resolved
assembly/src/parsers/ast/mod.rs Outdated Show resolved Hide resolved
assembly/src/parsers/ast/mod.rs Outdated Show resolved Hide resolved
assembly/src/parsers/ast/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I've added some comments inline.

assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! Thank you! I left a few comments inline.

assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/instruction/mod.rs Outdated Show resolved Hide resolved
assembly/src/todo/instruction/flow_ops.rs Outdated Show resolved Hide resolved
@bobbinth
Copy link
Contributor

bobbinth commented Nov 15, 2022

Some more thoughts about SpanBuilder struct. I think it could look something like this:

#[derive(Default)]
struct SpanBuilder {
    ops: Vec<Operation>,
    decorators: DecoratorList,
}

impl SpanBuilder {

    pub fn add_op(&mut self, op: Operation) -> Result<Option<CodeBlock>, AssemblyError> {
        self.ops.push(op);
        Ok(None)
    }

    pub fn add_ops(&mut self, ops: &[Operation]) -> Result<Option<CodeBlock>, AssemblyError> {
        ops.iter().for_each(|&op| self.ops.push(op));
        Ok(None)
    }

    pub fn push_op(&mut self, op: Operation) {
        self.ops.push(op);
    }

    pub fn has_ops(&self) -> bool {
        self.ops.is_empty()
    }

    pub fn extract_span_into(&mut self, target: &mut Vec<CodeBlock>) {
        if !self.ops.is_empty() {
            let ops: Vec<_> = self.ops.drain(..).collect();
            target.push(CodeBlock::new_span(ops));
        }
    }
}

The add_op() and add_ops() methods are intended to be used inside compile_instruction() like so:

pub(super) fn compile_instruction(
    &self,
    instruction: Instruction,
    span: &mut SpanBuilder,
    context: &AssemblyContext,
) -> Result<Option<CodeBlock>, AssemblyError> {
    match instruction {
        Instruction::Assert => span.add_op(Assert),
        Instruction::AssertEq => span.add_ops(&[Eq, Assert]),
        Instruction::Assertz => span.add_ops(&[Eqz, Assert]),
        Instruction::Add => span.add_op(Add),
        Instruction::AddImm(imm) => field_ops::add_imm(span, imm),
        ...
        Instruction::ExecLocal(idx) => flow_ops::exec_local(*idx, context),
        ...
    }
}

This way, for simple instructions we could add to the span directly in the match statement and remove quite a bit of boilerplate code. For more complex instructions, we'd still have specialized handler functions (e.g., as with the AddImm above).

@vlopes11 vlopes11 force-pushed the update-ast-to-compile-mast branch 2 times, most recently from 99dc8ec to eb76144 Compare November 15, 2022 23:05
@bobbinth bobbinth force-pushed the update-ast-to-compile-mast branch 4 times, most recently from a3abefb to 97b2494 Compare November 16, 2022 08:53
bobbinth and others added 24 commits November 18, 2022 18:23
eq and neq parsers, u32 error message
Implement handling of advice injectors in new assembler
Add functions and docs for AST parsers
Implement compilation of exp operations
Assembly module structure refactoring
Refactor AssemblyError and fix Miden executable
@bobbinth bobbinth marked this pull request as ready for review November 21, 2022 18:54
Copy link
Contributor

@bobbinth bobbinth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All looks good! Thank you, everyone - this was a big one!

@bobbinth bobbinth merged commit 584335b into next Nov 21, 2022
@bobbinth bobbinth deleted the update-ast-to-compile-mast branch November 21, 2022 18:59
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.

3 participants