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[venom]: add venom parser #4381

Open
wants to merge 14 commits into
base: master
Choose a base branch
from
Open

Conversation

Philogy
Copy link

@Philogy Philogy commented Nov 29, 2024

What I did

I added a "frontend" for venom, allowing you to compile independent venom files.

How I did it

I defined a new grammar (a bit simpler than what the readme initially suggested) & parser using the lark parser generator library.

How to verify it

Run the venom CLI against the example in the readme by putting it in a example.venom file and running venom example.venom

Commit message

feat[venom]: add frontend

Description for the changelog

Added independent CLI for compiling venom

Cute Animal Picture

image

Copy link

socket-security bot commented Nov 29, 2024

New and removed dependencies detected. Learn more about Socket for GitHub ↗︎

Package New capabilities Transitives Size Publisher
pypi/[email protected] eval, filesystem, unsafe 0 704 kB erez

View full report↗︎

@harkal
Copy link
Collaborator

harkal commented Nov 29, 2024

🚀

@harkal harkal changed the title feat[venom]: Add venom parser feat[venom]: add venom parser Nov 29, 2024
)
from vyper.venom.function import IRFunction
from lark import Lark, Transformer
from functools import reduce

Check notice

Code scanning / CodeQL

Unused import Note

Import of 'reduce' is not used.
@Philogy
Copy link
Author

Philogy commented Nov 30, 2024

@charles-cooper btw how I would I toggle optimizations on-off for faster venom compilation?

if args.evm_version is not None:
set_global_settings(Settings(evm_version=args.evm_version))

if args.input_file is None:
Copy link
Member

Choose a reason for hiding this comment

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

i'd rather have the unix convention of - indicating stdin, or maybe --stdin arg

bb.insert_instruction(instruction)

# Manually insert because we need to override function entry
fn._basic_block_dict[block_name] = bb
Copy link
Member

Choose a reason for hiding this comment

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

does fn.append_basic_block(bb) not work here?

Copy link
Author

Choose a reason for hiding this comment

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

No because when you initialize a basic block it automatically also creates the starting block so I want to be able to overwrite that


ctx.data_segment = data_section

ctx.chain_basic_blocks()
Copy link
Member

Choose a reason for hiding this comment

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

does chain_basic_blocks() need to be called? or should we assume the user input is well-formed?

Copy link
Author

Choose a reason for hiding this comment

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

I don't quite understand it but I think it's somewhat required to build the basic CFG and determine whether the CFGs terminate or not?

Copy link
Author

Choose a reason for hiding this comment

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

Oh wait, I think I found the issue "revert" is not considered a basic block terminator (not in vyper/venom/basicblock.py:BB_TERMINATORS) so that was giving me errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes, revert is not a terminator. You can have multiple reverts in a block and that is ok.

for fn_name, blocks in funcs:
fn = ctx.create_function(fn_name)
for block_name, instructions in blocks:
bb = IRBasicBlock(IRLabel(block_name), fn)
Copy link
Member

Choose a reason for hiding this comment

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

i think we might want to update ctx.last_label as well

Copy link
Author

Choose a reason for hiding this comment

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

last_label doesn't seem to be used anywhere?

fallback:
revert 0, 0
fn global => {
Copy link
Member

Choose a reason for hiding this comment

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

it's a little bit weird that functions are curly brace delimited but basic blocks are indent delimited

Copy link
Member

Choose a reason for hiding this comment

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

(not a huge deal, but it's something to think about -- either making basic blocks curly brace delimited or functions whitespace delimited)

Copy link
Author

Choose a reason for hiding this comment

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

tbh I like the mix, makes it clear which is which, if you really want can change

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I side with @charles-cooper on this. Seems weird to only have braces only for that part. Also is doesn't sit well with me as venom is more of a simple "assembly" like language, so no need for deep nesting.

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.

very slick -- much simpler than i was expecting! left a few comments

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