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

Simplify Expr.arithmetic #1661

Merged
merged 6 commits into from Nov 11, 2019
Merged

Conversation

@charles-cooper
Copy link
Collaborator

charles-cooper commented Oct 26, 2019

What I did

  • Fix #1658
  • Always precompute both sides into an LLL reference (pre_alloc for call
    no longer needed), so generally more gas efficient if the arguments
    are more than ~2 ops
  • Remove a jumpi for mul uint256
  • Simplify type calculation / pos calculation
  • Use clamps instead of seq-assert where it's clearer
  • Use similar styles for similar output (e.g. mul uint256 looks like mul
    int128)

How I did it

Use references to break the exponential behavior in #1658

How to verify it

Description for the changelog

Cute Animal Picture

Put a link to a cute animal picture inside the parenthesis-->

@charles-cooper

This comment has been minimized.

Copy link
Collaborator Author

charles-cooper commented Oct 26, 2019

and of course that breaks constant folding.

@jacqueswww

This comment has been minimized.

Copy link
Collaborator

jacqueswww commented Oct 28, 2019

@charles-cooper it's not breaking constant folding, you just have to fix the test to extract the constant value differently now that the LLL output has changed (which makes sens IMO) ;)

the safe add now generates different code, so just look for the return
statement.
- Always precompute both sides into an LLL reference (pre_alloc for call
    no longer needed), so generally more gas efficient if the arguments
    are more than ~2 ops
- Remove a jumpi for mul uint256
- Simplify type calculation / pos calculation
- Use clamps instead of seq-assert where it's clearer
- Use similar styles for similar output (e.g. mul uint256 looks like mul
    int128)
@charles-cooper charles-cooper changed the title WIP use references for adding Simplify Expr.arithmetic Oct 31, 2019
Fix lint
adhere to pylint's draconian whitespace rules
Copy link
Member

fubuloubu left a comment

Looks cleaner, let's fix the TODOs or at least have a discussion about them

vyper/parser/expr.py Show resolved Hide resolved
vyper/parser/expr.py Outdated Show resolved Hide resolved
Copy link
Collaborator

jacqueswww left a comment

Looks good @charles-cooper just add the missing overflow case (or remove the comment) - and I believe we can merge this :)

@fubuloubu fubuloubu merged commit e7e26e4 into vyperlang:master Nov 11, 2019
3 checks passed
3 checks passed
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: py36-core Your tests passed on CircleCI!
Details
ci/circleci: py37-core Your tests passed on CircleCI!
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.