Skip to content

Conversation

harkal
Copy link
Collaborator

@harkal harkal commented Oct 1, 2025

What I did

How I did it

How to verify it

Commit message

Commit message for the final, squashed PR. (Optional, but reviewers will appreciate it! Please see our commit message style guide for what we would ideally like to see in a commit message.)

Description for the changelog

Cute Animal Picture

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

AssignElimination(ac, fn).run_pass()
CSE(ac, fn).run_pass()
def _run_passes(fn: IRFunction, flags: VenomOptimizationFlags, ac: IRAnalysesCache) -> None:
passes = OPTIMIZATION_PASSES.get(flags.level, OPTIMIZATION_PASSES[OptimizationLevel.O2])
Copy link
Member

Choose a reason for hiding this comment

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

probably better to fail with a key lookup error since it indicates a mistake in the optimization passes dict (or how we have constructed flags.level)

if field.name == "venom_flags":
# Skip venom_flags - we'll handle it after optimization level is merged
continue
else:
Copy link
Member

Choose a reason for hiding this comment

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

can dedent this - no need for else since the if branch hits continue

Comment on lines +259 to +260
if args.no_inlining:
settings.venom_flags.disable_inlining = True
Copy link
Member

@charles-cooper charles-cooper Oct 6, 2025

Choose a reason for hiding this comment

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

can we do it like this?

Suggested change
if args.no_inlining:
settings.venom_flags.disable_inlining = True
settings.venom_flags.disable_inlining = args.no_inlining


# Disable flags - default False means optimization is enabled
# These are used to override the defaults for the optimization level
disable_inlining: bool = False
Copy link
Member

Choose a reason for hiding this comment

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

not sure there is anything to be done atm but i am concerned about long term drift between these fields and the relevant code in vyper_compile.py and vyper_json.py

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.

2 participants