refactor[codegen]: split codegen packages#4925
Conversation
Move source tracking (ast_source/error_msg stacks) from IRFunction to VenomBuilder. IRInstruction.ast_source is now correctly typed as Optional[VyperNode] instead of Optional[IRnode]. Builder passes source info explicitly to append_instruction/insert_instruction instead of IRBasicBlock reading it from IRFunction. This removes the last dependency of vyper/venom/ on vyper/codegen/ir_node.
…codegen_shared Rename vyper/codegen/ to vyper/codegen_legacy/ to make room for venom as the default codegen. Extract shared utilities into vyper/codegen_shared/: - abi_utils.py: is_tuple_like, needs_external_call_wrap, calculate_type_for_external_return, abi_encoding_matches_vyper, DYNAMIC_ARRAY_OVERHEAD - arithmetic.py: calculate_largest_base, calculate_largest_power - function_info.py: _FuncIRInfo, EntryPointInfo, init_ir_info - jumptable_utils.py: selector dispatch utilities codegen_venom now imports from codegen_shared instead of codegen_legacy. All references to vyper.codegen updated to vyper.codegen_legacy.
Venom codegen is now the default. Use --legacy or pragma legacy to use the old IRnode-based codegen. The --experimental-codegen and --venom-experimental flags are deprecated (they still work but are hidden from --help). CI matrix updated: default tests use venom, legacy tests use --legacy. Fuzzing runs on venom by default.
Support 'legacy: true' in the JSON settings, as a counterpart to the CLI --legacy flag. When set, forces the legacy IRnode codegen.
…antics) Settings.experimental_codegen -> Settings.legacy_codegen. True = legacy IRnode codegen, False (default) = Venom. CLI: --legacy sets legacy_codegen=True. --experimental-codegen still works (deprecated, hidden). Pragmas: pragma legacy -> legacy_codegen=True. pragma experimental-codegen still works (deprecated). JSON API: legacyCodegen/legacy key supported. experimentalCodegen still works (inverted: True -> legacy_codegen=False). All internal checks flipped: 'if experimental_codegen' -> 'if not legacy_codegen'. Test fixture 'experimental_codegen' kept for compat (returns True=venom).
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| from vyper.ast.validation import validate_call_args | ||
| from vyper.codegen.expr import Expr | ||
| from vyper.codegen.ir_node import IRnode | ||
| from vyper.codegen_legacy.expr import Expr |
| from vyper.codegen.abi_encoder import abi_encode | ||
| from vyper.codegen.context import Context | ||
| from vyper.codegen.core import ( | ||
| from vyper.codegen_legacy.abi_encoder import abi_encode |
|
|
||
| from vyper.codegen.core import bytes_data_ptr, ensure_in_memory, get_bytearray_length | ||
| from vyper.codegen.ir_node import IRnode | ||
| from vyper.codegen_legacy.core import bytes_data_ptr, ensure_in_memory, get_bytearray_length |
| from vyper import ast as vy_ast | ||
| from vyper.codegen.core import _freshname, eval_once_check, make_setter | ||
| from vyper.codegen.ir_node import IRnode | ||
| from vyper.codegen_legacy.core import _freshname, eval_once_check, make_setter |
|
|
||
| def ir_for_self_call(stmt_expr: vy_ast.Call, context): | ||
| from vyper.codegen.expr import Expr # TODO rethink this circular import | ||
| from vyper.codegen_legacy.expr import Expr # TODO rethink this circular import |
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0498268e04
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| legacy_codegen = input_dict["settings"].get("legacyCodegen") | ||
| if legacy_codegen is None: | ||
| legacy_codegen = input_dict["settings"].get("legacy") |
There was a problem hiding this comment.
This adds legacyCodegen/legacy parsing, but get_output_formats() still decides whether cfg/cfg_runtime are allowed using only deprecated venomExperimental/experimentalCodegen keys. As a result, JSON inputs like {"settings": {"legacyCodegen": false, "outputSelection": {"*": ["cfg_runtime"]}}} still fail with “experimentalCodegen not selected” even though venom is explicitly selected, making the new settings API internally inconsistent.
Useful? React with 👍 / 👎.
Module.py reads global _settings via _opt_none()/_opt_codesize(), but generate_venom_runtime/deploy never anchored settings. This caused AttributeError when _settings was None.
Tests for asm optimizer DCE and selector table stability assume legacy codegen output but didn't specify legacy_codegen=True. Now that venom is default, they need explicit opt-in. Also fix Black formatting in pre_parser.py and test_liveness_simple_loop.py.
| code, output_formats=["asm_runtime"], settings=Settings(optimize=OptimizationLevel.CODESIZE) | ||
| code, | ||
| output_formats=["asm_runtime"], | ||
| settings=Settings(optimize=OptimizationLevel.CODESIZE, legacy_codegen=True), |
There was a problem hiding this comment.
Does this test only work on the legacy codegen, and if so, why ?
| Check all examples round trip | ||
| """ | ||
| if not compiler_settings.experimental_codegen: | ||
| if not compiler_settings.legacy_codegen: |
There was a problem hiding this comment.
| if not compiler_settings.legacy_codegen: | |
| if compiler_settings.legacy_codegen: |
At least to correspond to the text
| Test vyper_sources round trip | ||
| """ | ||
| if not compiler_settings.experimental_codegen: | ||
| if not compiler_settings.legacy_codegen: |
There was a problem hiding this comment.
| if not compiler_settings.legacy_codegen: | |
| if compiler_settings.legacy_codegen: |
same as above
| @pytest.mark.parametrize("code", codes) | ||
| def test_dead_code_eliminator(code): | ||
| c = CompilerData(code, settings=Settings(optimize=OptimizationLevel.NONE)) | ||
| c = CompilerData(code, settings=Settings(optimize=OptimizationLevel.NONE, legacy_codegen=True)) |
| """ | ||
|
|
||
| settings = Settings(experimental_codegen=True, optimize=OptimizationLevel.O3) | ||
| settings = Settings(legacy_codegen=False, optimize=OptimizationLevel.O3) |
There was a problem hiding this comment.
Default, so unneeded ?
(applies to others below)
| ) | ||
| parser.addoption("--enable-compiler-debug-mode", action="store_true") | ||
| parser.addoption("--experimental-codegen", action="store_true") | ||
| parser.addoption("--experimental-codegen", action="store_true", default=True) |
There was a problem hiding this comment.
Should be removed/deprecated, no ?
# Conflicts: # .github/workflows/test.yml # vyper/codegen/function_definitions/common.py
Gas ChangesNo changes detected. Summary
|
|
is |
|
Stale docs/strings still recommend the deprecated flag |
|
|
|
related: #4862 |
|
i think this should be split into two PRs, the refactor PR and making venom the default |
# Conflicts: # tests/functional/builtins/codegen/test_convert.py
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4925 +/- ##
==========================================
+ Coverage 92.92% 92.94% +0.01%
==========================================
Files 188 191 +3
Lines 27818 27816 -2
Branches 4827 4827
==========================================
+ Hits 25851 25854 +3
+ Misses 1316 1311 -5
Partials 651 651 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e1d3b49f59
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| from vyper.ast import natspec | ||
| from vyper.codegen import module | ||
| from vyper.codegen.ir_node import IRnode | ||
| from vyper.codegen_legacy import module |
There was a problem hiding this comment.
For callers that do not pass --experimental-codegen or a pragma, CompilerData.settings still turns an unset experimental_codegen into False, and this renamed import keeps generate_ir_nodes() wired to codegen_legacy.module.generate_ir_for_module. As a result the default API/CLI path still produces vyper.codegen_legacy.ir_node.IRnode output and bytecode from the legacy pipeline; Venom is only used when explicitly opted in, so the new default codegen behavior is not actually enabled.
Useful? React with 👍 / 👎.
charles-cooper
changed the title
| self._source_code, | ||
| node.target.lineno, | ||
| node.target.col_offset, | ||
| ) |
There was a problem hiding this comment.
Did this sneak in from a different PR ?
There was a problem hiding this comment.
b4145b7 this one but it seems like a diff viewing issue
5 participants
co-authored by claude opus 4.6
What I did
Split the legacy IRnode codegen package from helpers shared with the direct Venom codegen pipeline.
This PR no longer changes the default codegen pipeline: legacy remains the default, and Venom remains opt-in via the existing experimental codegen setting and CLI/API flags.
How I did it
Renamed
vyper/codegen/→vyper/codegen_legacy/— legacy IRnode codegen remains isolated under the new package name, with imports updated across the codebase.Created
vyper/codegen_shared/— ABI helpers, safe arithmetic bounds, function metadata, and selector table utilities now live in a shared package used by both codegen pipelines.Decoupled Venom source tracking from IRnode —
IRFunctionno longer owns codegen source/error stacks;VenomBuilderdoes.IRInstruction.ast_sourceis typed asOptional[VyperNode], andvyper/venom/no longer imports legacy codegen types.How to verify it
Commit message
the legacy IRnode codegen and direct Venom codegen now share several
concepts, but the old
vyper/codegenpackage mixed legacy-only IRnodeimplementation details with helpers that both pipelines need. That made
shared code hard to identify and caused
vyper/venomto depend onlegacy codegen types for source tracking.
split the legacy pipeline into
vyper/codegen_legacyand move commonABI, arithmetic, function metadata, and selector table helpers into
vyper/codegen_shared. The Venom pipeline imports those shared helpersdirectly instead of reaching through legacy codegen.
also move source/error tracking ownership from
IRFunctiontoVenomBuilder, so source attribution remains a codegen concern andIRInstruction.ast_sourcecan hold the frontendVyperNodedirectly.this is intentionally a refactor-only change: legacy codegen remains the
default pipeline and Venom remains opt-in via the existing experimental
codegen setting and CLI/API flags. The default-pipeline switch is left
for a follow-up PR.
Description for the changelog
Split legacy codegen into
vyper/codegen_legacyand move shared helpers intovyper/codegen_shared. Venom remains opt-in.Cute Animal Picture