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

Basic Yul compiler #16

Merged
merged 5 commits into from
Aug 25, 2020
Merged

Basic Yul compiler #16

merged 5 commits into from
Aug 25, 2020

Conversation

g-r-a-n-t
Copy link
Member

@g-r-a-n-t g-r-a-n-t commented Feb 17, 2020

Part of #10.

This includes just enough functionality to compile a contract like this:

contract Foo:
    pub def bar(x: u256) -> u256:
        return x

To keep it simple, types are basically ignored (everyting is u256) and all variables are stored on the stack. There are some Scope data structures in here as well, which aren't necessary for it to work, but would be worth reviewing as I intend to build more complexity on top of them.

The Yul code generated by the compiler would not actually be runnable, since it doesn't have the ABI builder from #15 to generate the selector switch from. In #17, I add that to this module and demonstrate that the contract can be deployed and ran in a test.

@g-r-a-n-t g-r-a-n-t changed the title WIP: Basic Yul compiler Basic Yul compiler Feb 20, 2020
Copy link
Member

@ralexstokes ralexstokes left a comment

Choose a reason for hiding this comment

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

nice work! i like how you have broken a big chunk of work into digestible PRs... looking forward to seeing it all come together :)

i left some comments but overall seems like a good foundation for extending the compiler

[dependencies]
vyper-parser = {path = "../parser", version = "0.1.0"}
hex = "0.4"
yultsur = { git = "https://github.com/g-r-a-n-t/yultsur" }
Copy link
Member

Choose a reason for hiding this comment

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

anything special in this fork of yultsur compared to the one maintained by the solidity devs?

Copy link
Member Author

Choose a reason for hiding this comment

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

I needed to add a few structs that exist in the Yul spec, but not in yultsur.

let tokens = parser::get_parse_tokens(src)?;
let vyp_module = parser::parsers::file_input(&tokens[..])?.1.node;

// TODO: Handle multiple contracts in one Vyper module cleanly.
Copy link
Member

Choose a reason for hiding this comment

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

note: resolved in future PRs

Comment on lines +23 to +32
yul::Expression::FunctionCall(yul::FunctionCall {
identifier: base::untyped_identifier("shr"),
arguments: vec![
base::untyped_literal_expr("224"),
yul::Expression::FunctionCall(yul::FunctionCall {
identifier: base::untyped_identifier("calldataload"),
arguments: vec![base::untyped_literal_expr("0")],
}),
],
})
Copy link
Member

Choose a reason for hiding this comment

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

"leave the first 4 bytes (big-endian) of the calldata on the stack"

keccak.finalize(&mut selector);
}

base::untyped_literal(&format!("0x{}", hex::encode(selector)))
Copy link
Member

Choose a reason for hiding this comment

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

i'd have to dig into yul further but i wonder if we can tighten the typing here? "untyped" literal may be the strictest thing we can specify given a hex string -- curious if you can add any color here...

Copy link
Member Author

@g-r-a-n-t g-r-a-n-t May 12, 2020

Choose a reason for hiding this comment

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

This is what it looks like now:

literal! { (format!("0x{}", hex::encode(selector))) }

See the literal macro. (I should add better docs to that file, as the notation is somewhat confusing.)

Currently, solc's yul compiler doesn't even handle explicit types. And even if it did, it probably wouldn't be very useful, so I just removed the prefix altogether.

Copy link
Member

Choose a reason for hiding this comment

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

cool cool

block: yul::Block {
statements: vec![yul::Statement::Expression(yul::Expression::FunctionCall(
yul::FunctionCall {
identifier: base::untyped_identifier("foo"),
Copy link
Member

Choose a reason for hiding this comment

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

might not need to update here if this is temporary but i expect we can grab some identifier from the ethabi::Function

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh, woops. This got fixed later on, probably after trying to run a function not named "foo".

Comment on lines +57 to +70
Ok(module
.body
.iter()
.map(|stmt| module_stmt(Rc::clone(&scope), &stmt.node))
.collect::<Result<Vec<Option<yul::Statement>>, CompileError>>()?
.into_iter()
.filter_map(|statement| {
if let Some(yul::Statement::Object(object)) = statement {
return Some(object);
}

None
})
.collect::<Vec<yul::Object>>())
Copy link
Member

Choose a reason for hiding this comment

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

i would expect this can be simplified like over here: #15 (comment)

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'll remove this pattern wherever I see it.

Comment on lines +74 to +89
pub fn module_stmt<'a>(
scope: Shared<ModuleScope<'a>>,
stmt: &'a vyp::ModuleStmt<'a>,
) -> Result<Option<yul::Statement>, CompileError> {
match stmt {
vyp::ModuleStmt::TypeDef { .. } => {
type_def(scope, stmt)?;
Ok(None)
}
vyp::ModuleStmt::ContractDef { .. } => {
contract_def(scope, stmt).map(|object| Some(yul::Statement::Object(object)))
}
_ => Err(CompileError::static_str(
"Unable to translate module statement.",
)),
}
Copy link
Member

Choose a reason for hiding this comment

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

seems like the uses of Option here can just be cases of CompileError and then it simplifies much of the control flow...

.collect::<Result<Vec<yul::Statement>, CompileError>>()?;

// TODO: Use functions from actual contract ABI once the builder is merged.
statements.push(yul::Statement::Switch(selectors::switch(vec![])?));
Copy link
Member

Choose a reason for hiding this comment

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

see comment under selectors module about this

Suggested change
statements.push(yul::Statement::Switch(selectors::switch(vec![])?));
statements.push(yul::Statement::Switch(selectors::dispatch_switch(vec![])?));

@@ -0,0 +1,175 @@
use crate::errors::CompileError;
Copy link
Member

Choose a reason for hiding this comment

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

something that came across my mind when reviewing this was the genericity of the function names switch, expression and case. you could make the argument that we rebind the names on import elsewhere and they make sense in the context of this module, but it is something to consider.

i left a comment at a usage of switch to point out what I mean

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree. There must be applicable CS nomenclature for this.

Copy link
Member

Choose a reason for hiding this comment

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

i'd just go with what sounds natural and readable. and ideally unique across as many contexts as possible. of course, we quickly end up optimizing a many-dimensional space and can't satisfy all of our constraints at once :)

off the top of my head, i would call the thing expression as predicate as the selector::predicate, while still jargon-y is clearer to me which part of the expression it is referring to, rather than just selector::expression.

and i'd prolly add some more semantic content to the symbol for case -- something like switch_case or arm or something.... this being said if these functions don't leak outside of this module there is less need to do this given the context gained as a local reader of this module

}

/// Builds an expression that loads the first 4 bytes of the calldata.
pub fn expression() -> yul::Expression {
Copy link
Member

Choose a reason for hiding this comment

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

do we need anything pub in this module except switch?

Copy link
Member Author

Choose a reason for hiding this comment

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

@ralexstokes
Copy link
Member

thanks for the feedback! looks good to me :)

@g-r-a-n-t g-r-a-n-t merged commit e54a151 into ethereum:master Aug 25, 2020
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.

None yet

2 participants