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

feat: remove deploy instruction from venom #3703

Conversation

harkal
Copy link
Collaborator

@harkal harkal commented Dec 20, 2023

What I did

Venom IR for an easier transition employed a deploy instruction like the old IR. This was creating friction in the new system as it was implicitly dictating basic block flows, that had to be handled with special cases code.

This PR removes the need for the deploy instruction. The original IR is split into deploy and runtime code as soon as possible, and the two parts are handed to the new backend, which now handles any number of contexts that it links when emitting assembly.

How I did it

How to verify it

Commit message

Removes the need for the deploy instruction

  • Venom IR now does not support the deploy instruction
  • Makes the venom backend handle multiple contexts (functions to be linked together)
  • Adds multiple entry points to contexts

Description for the changelog

Cute Animal Picture

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

adopted a shorted name for frequently used methods
Refactored append instructions into a single append_instruction() method
with named arguments for returning or not a variable
Until now log instructions where pass through. Now we have a log instruction
in venom that takes the first argument to be the topic count
(in shape of doing table jmp etc in the future -essentialy have
instructions that require manipulation of operants before emit)
Remove output argument append_instruction() in IRBasicBlock
now the method decides by itself if there is an output value
vyper/venom/__init__.py Fixed Show fixed Hide fixed
vyper/venom/__init__.py Fixed Show fixed Hide fixed
vyper/venom/ir_node_to_venom.py Fixed Show fixed Hide fixed
@codecov-commenter
Copy link

codecov-commenter commented Dec 20, 2023

Codecov Report

Attention: 52 lines in your changes are missing coverage. Please review.

Comparison is base (3116e88) 83.99% compared to head (fe0e154) 84.12%.
Report is 8 commits behind head on master.

Files Patch % Lines
vyper/venom/ir_node_to_venom.py 8.00% 23 Missing ⚠️
vyper/venom/venom_to_assembly.py 50.00% 15 Missing and 2 partials ⚠️
vyper/venom/__init__.py 53.84% 5 Missing and 1 partial ⚠️
vyper/compiler/phases.py 55.55% 3 Missing and 1 partial ⚠️
vyper/venom/function.py 85.71% 2 Missing ⚠️

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3703      +/-   ##
==========================================
+ Coverage   83.99%   84.12%   +0.13%     
==========================================
  Files          92       92              
  Lines       13056    13034      -22     
  Branches     2932     2919      -13     
==========================================
- Hits        10966    10965       -1     
+ Misses       1658     1649       -9     
+ Partials      432      420      -12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@harkal harkal marked this pull request as ready for review December 21, 2023 13:29
@@ -19,17 +19,14 @@


def generate_assembly_experimental(
ctx: IRFunction, optimize: Optional[OptimizationLevel] = None
ctxs: tuple[IRFunction, IRFunction], optimize: Optional[OptimizationLevel] = None
Copy link
Member

Choose a reason for hiding this comment

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

this should either be a list of IRFunctions or specify by names -- runtime_ir and deploy_ir.

@@ -75,13 +77,13 @@
# with the assembler. My suggestion is to let this be for now, and we can
# refactor it later when we are finished phasing out the old IR.
class VenomCompiler:
ctx: IRFunction
ctxs: list[IRFunction]
Copy link
Member

Choose a reason for hiding this comment

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

should these still be named ctxs? maybe functions?

Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

seems fine, but not a fan of the new API for generate_assembly_experimental. (ditto for VenomCompiler -- i think it should not allow None functions as part of its input.

@harkal
Copy link
Collaborator Author

harkal commented Dec 23, 2023

seems fine, but not a fan of the new API for generate_assembly_experimental. (ditto for VenomCompiler -- i think it should not allow None functions as part of its input.

generate_assembly_experimental() is just glue code for the old-new code interface. It's basic purpose is to "untangle" the combinations of ir_runtime and ir_nodes properties that depending on the run parameters can be multiple things (one of them being the (None, IRnode) tuple in the case of doing runtime only code.

VenomCompiler() takes just a list of IRFunctions to compile, which is clean. With the last commit I removed the None check inside VenomCompiler for cleaning up the list of Nones in the glue code which is indeed nicer.

@charles-cooper charles-cooper changed the title feat: venom deploy instruction removal feat: remove deploy instruction from venom Jan 2, 2024
@charles-cooper charles-cooper enabled auto-merge (squash) January 2, 2024 16:46
Copy link
Member

@charles-cooper charles-cooper left a comment

Choose a reason for hiding this comment

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

nice! i made some changes as discussed offline, please take a look and see if you like them. i'll merge for now and if you have any changes you want to make we can do so in a follow up PR

@charles-cooper charles-cooper merged commit 0c82d0b into vyperlang:master Jan 2, 2024
84 checks passed
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