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

Lowering pass and augmented assignments #338

Merged
merged 1 commit into from
Mar 30, 2021

Conversation

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

@g-r-a-n-t g-r-a-n-t commented Mar 25, 2021

Added a lowering pass to the compiler module and used it to support augmented assignments.

e.g. foo += 1 becomes foo = foo + 1

To-Do

  • OPTIONAL: Update Spec if applicable

  • Add entry to the release notes (may forgo for trivial changes)

  • Clean up commit history

@g-r-a-n-t g-r-a-n-t marked this pull request as draft March 25, 2021 18:48
compiler/src/lowering/mappers/contracts.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/expressions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/functions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/functions.rs Outdated Show resolved Hide resolved
@g-r-a-n-t g-r-a-n-t mentioned this pull request Mar 26, 2021
@codecov-io
Copy link

codecov-io commented Mar 26, 2021

Codecov Report

Merging #338 (815e4bf) into master (dd017d8) will increase coverage by 0.16%.
The diff coverage is 98.37%.

❗ Current head 815e4bf differs from pull request most recent head 4f4f5a8. Consider uploading reports for the commit 4f4f5a8 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #338      +/-   ##
==========================================
+ Coverage   92.60%   92.77%   +0.16%     
==========================================
  Files          56       59       +3     
  Lines        3882     4000     +118     
==========================================
+ Hits         3595     3711     +116     
- Misses        287      289       +2     
Impacted Files Coverage Δ
analyzer/src/traversal/assignments.rs 96.66% <ø> (ø)
compiler/src/yul/mappers/expressions.rs 90.19% <ø> (ø)
compiler/src/lowering/mappers/module.rs 75.00% <75.00%> (ø)
analyzer/src/traversal/functions.rs 97.41% <100.00%> (+0.02%) ⬆️
compiler/src/lib.rs 90.32% <100.00%> (+0.32%) ⬆️
compiler/src/lowering/mappers/expressions.rs 100.00% <100.00%> (ø)
compiler/src/lowering/mappers/functions.rs 100.00% <100.00%> (ø)
compiler/src/lowering/mod.rs 100.00% <100.00%> (ø)
parser/src/node.rs 80.95% <100.00%> (+3.17%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dd017d8...4f4f5a8. Read the comment docs.

@g-r-a-n-t g-r-a-n-t force-pushed the lowering-pass-aug-assign branch 2 times, most recently from 548d5f9 to fb0f056 Compare March 26, 2021 22:32
analyzer/src/traversal/assignments.rs Show resolved Hide resolved
compiler/src/lowering/mappers/contracts.rs Show resolved Hide resolved
compiler/src/lowering/mappers/expressions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/expressions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mappers/expressions.rs Show resolved Hide resolved
unreachable!()
}

fn func_stmt(context: &Context, stmt: Node<fe::FuncStmt>) -> Vec<Node<fe::FuncStmt>> {
Copy link
Member Author

Choose a reason for hiding this comment

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

Note: returning a vec here because we'll have lowerings like:

(foo, bar): (u256, u256) = my_tuple

to

foo: u256 = my_tuple.item0
bar: u256 = my_tuple.item1

which requires that we be able to map a single statement to multiple statements.

Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a good candidate for SmallVec<[Node<..>, 1]> in some possible future performance optimization.

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense. I'll track this in an issue.

compiler/src/lowering/mappers/functions.rs Outdated Show resolved Hide resolved
compiler/src/lowering/mod.rs Outdated Show resolved Hide resolved
parser/src/node.rs Show resolved Hide resolved
src/main.rs Show resolved Hide resolved
@@ -278,7 +278,7 @@ pub fn expr_bin_operation(context: &Context, exp: &Node<fe::Expr>) -> yul::Expre
}
_ => unreachable!(),
},
_ => unimplemented!(),
fe::BinOperator::FloorDiv => unimplemented!(),
Copy link
Member Author

Choose a reason for hiding this comment

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

see #341

@@ -0,0 +1,57 @@
contract Foo:
Copy link
Member Author

Choose a reason for hiding this comment

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

It would be nice to have some tests that check that source files are correctly lowered. We need to be able to pretty-print ASTs for this, tho. See #326.

@g-r-a-n-t g-r-a-n-t marked this pull request as ready for review March 29, 2021 19:54
@g-r-a-n-t g-r-a-n-t requested a review from sbillig March 29, 2021 20:45
value,
} = &stmt.kind
{
let target_attributes = expressions::expr(Rc::clone(&scope), Rc::clone(&context), target)?;
Copy link
Member Author

Choose a reason for hiding this comment

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

relevant: #345


/// Lowers a boxed expression.
#[allow(clippy::boxed_local)]
pub fn boxed_expr(context: &Context, exp: Box<Node<fe::Expr>>) -> Box<Node<fe::Expr>> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is another pattern that might be worth optimizing in the future, or might not matter; we take a boxed expr, probably don't modify it, and rebox. An extra allocation for every expr.

Copy link
Collaborator

@sbillig sbillig 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. My only (mild) concern is potential performance issues with all the allocations happening. Probably won't matter in practice, and if it does we can work on that later. (See inline comments above).

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.

4 participants