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

fix: optimizer regression #2868

Merged
merged 7 commits into from
May 27, 2022

Conversation

charles-cooper
Copy link
Member

What I did

fix overly aggressive optimizations introduced in #2647

How I did it

roll back the optimization if a side-effecting operation is not found in the optimized result. the heuristic is overly conservative, which i am ok with.

How to verify it

see code, tests

Commit message

Description for the changelog

Cute Animal Picture

image

the optimizer passes introduced in efe1dbe were too aggressive, and
would optimize out expressions that had side effects.
@codecov-commenter
Copy link

codecov-commenter commented May 23, 2022

Codecov Report

Merging #2868 (f3a8237) into master (3195701) will increase coverage by 0.02%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #2868      +/-   ##
==========================================
+ Coverage   87.67%   87.70%   +0.02%     
==========================================
  Files          94       94              
  Lines       10183    10214      +31     
  Branches     2501     2509       +8     
==========================================
+ Hits         8928     8958      +30     
  Misses        790      790              
- Partials      465      466       +1     
Impacted Files Coverage Δ
vyper/codegen/ir_node.py 92.80% <100.00%> (ø)
vyper/ir/optimizer.py 91.72% <100.00%> (+0.32%) ⬆️
vyper/codegen/external_call.py 97.24% <0.00%> (-0.83%) ⬇️
vyper/compiler/output.py 93.78% <0.00%> (ø)
vyper/ir/compile_ir.py 93.69% <0.00%> (+0.06%) ⬆️
vyper/semantics/validation/local.py 91.13% <0.00%> (+0.27%) ⬆️

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 3195701...f3a8237. Read the comment docs.

@charles-cooper
Copy link
Member Author

The reason I am leaving this as a draft is because if we really want to fix the optimizer here, we need to get a bit more principled. Basically, we need an effects graph and machinery to ensure we don't break the effects graph. So, if we have other priorities besides the optimizer, an alternative is to rollback #2647 and leave this optimizer project for later.

@charles-cooper charles-cooper marked this pull request as ready for review May 26, 2022 10:36
@charles-cooper charles-cooper requested a review from fubuloubu May 26, 2022 11:50
@charles-cooper charles-cooper merged commit f5051e1 into vyperlang:master May 27, 2022
@charles-cooper charles-cooper deleted the fix/optimizer branch May 27, 2022 20:52
@emc415 emc415 mentioned this pull request Dec 19, 2022
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