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 boolops #2467

Merged
merged 4 commits into from
Oct 13, 2021
Merged

Conversation

charles-cooper
Copy link
Member

@charles-cooper charles-cooper commented Oct 1, 2021

What I did

Removed unnecessary goto and seq_unchecked from bool ops. Also has the added benefit of making the code more efficient. The following code loses 6 bytes worth of bytecode:

@external
def _and(x: bool, y: bool, z: bool) -> bool:
    return x and y and z

@external
def _or(x: bool, y: bool, z: bool) -> bool:
    return x or y or z

The difference in IR for the x or y or z expression is

before:

[mstore,
  0,
  [seq_unchecked,
    [if,
      [calldataload, 4 <x>],
      [1, [goto, _boolop_146:11:0]],
      [if,
        [calldataload, 36 <y>],
        [1, [goto, _boolop_146:11:0]],
        [calldataload, 68 <z>]]],
    [label, _boolop_146:11:0]]],

after:

[mstore,
  0,
  [if,
    [iszero, [calldataload, 4 <x>]],
    [if, [iszero, [calldataload, 36 <y>]], [calldataload, 68 <z>], 1],
    1]],

The difference for the x and y and z expression is similar.

How to verify it

Tests still pass, also compile the above example

Description for the changelog

Simplify code generation for boolean operations

Cute Animal Picture

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

@codecov-commenter
Copy link

codecov-commenter commented Oct 1, 2021

Codecov Report

Merging #2467 (ecbf63f) into master (39036fb) will decrease coverage by 0.16%.
The diff coverage is 100.00%.

❗ Current head ecbf63f differs from pull request most recent head 17c1d6e. Consider uploading reports for the commit 17c1d6e to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2467      +/-   ##
==========================================
- Coverage   84.52%   84.35%   -0.17%     
==========================================
  Files          93       93              
  Lines        9228     9271      +43     
  Branches     2085     2102      +17     
==========================================
+ Hits         7800     7821      +21     
- Misses        939      954      +15     
- Partials      489      496       +7     
Impacted Files Coverage Δ
vyper/lll/optimizer.py 82.80% <100.00%> (+0.56%) ⬆️
vyper/old_codegen/expr.py 79.10% <100.00%> (-0.17%) ⬇️
vyper/old_codegen/parser_utils.py 79.46% <0.00%> (-3.60%) ⬇️
vyper/exceptions.py 92.59% <0.00%> (-0.07%) ⬇️
vyper/ast/nodes.py 93.58% <0.00%> (ø)
vyper/cli/utils.py 92.10% <0.00%> (ø)
vyper/ast/natspec.py 98.76% <0.00%> (ø)
vyper/evm/opcodes.py 100.00% <0.00%> (ø)
vyper/ast/expansion.py 94.11% <0.00%> (ø)
... and 40 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 39036fb...17c1d6e. Read the comment docs.

@fubuloubu
Copy link
Member

Does this still retain short-circuiting properties?

@charles-cooper
Copy link
Member Author

charles-cooper commented Oct 2, 2021

FYI - this introduces an extra jump for the nested operations, each block jumps out to the enclosing block. For the most nested block it will do something like

if ... {
  if ... {
    if ... {
      goto if_exit1
    }
    label if_exit1
    goto if_exit2
  }
  label if_exit2
  goto if_exit3
}
label if_exit3

We can analyze and optimize it out of the asm. Basically if the destination of a jump is another jump, we just jump to the final destination. But maybe I'll include that analysis in this PR before it's merged

@charles-cooper
Copy link
Member Author

Does this still retain short-circuiting properties?

And yes. If we don't have a test for that, we should

@fubuloubu
Copy link
Member

Does this still retain short-circuiting properties?

And yes. If we don't have a test for that, we should

I'm pretty sure we do, I just don't know where.

get rid of the unnecessary goto and seq_unchecked
this makes it easier to inspect the assembly after it has been
optimized.
`if (x) (IF_TRUE) (IF_FALSE)` is more efficiently expressed as
`if (iszero(x)) (IF_FALSE) (IF_TRUE)`
@charles-cooper charles-cooper force-pushed the simplify_boolop branch 2 times, most recently from 35d273c to 17c1d6e Compare October 13, 2021 20:50
also fixes the existing optimization which didn't properly modify the
argument in place.

this also collapses sequences of JUMPs. this makes exiting out of nested
blocks (e.g. nested if statements) more efficient.
@charles-cooper
Copy link
Member Author

We can analyze and optimize it out of the asm. Basically if the destination of a jump is another jump, we just jump to the final destination. But maybe I'll include that analysis in this PR before it's merged

done in f6155f7

@charles-cooper charles-cooper merged commit 12a8b6a into vyperlang:master Oct 13, 2021
@charles-cooper charles-cooper deleted the simplify_boolop branch October 14, 2021 00:11
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