fix[venom]: fix various well formedness issues - #5231
HodanPlodky wants to merge 33 commits into
Conversation
Gas ChangesNo changes detected. Summary
|
📊 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: ba12d1bb46
ℹ️ 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".
| # host's own reclaim governs the inlined data. Whether the | ||
| # host publishes is determined solely by the host's own | ||
| # terminators (plain `ret` is callee-save). | ||
| assert retpc_op in inst.operands, retpc_op |
There was a problem hiding this comment.
When a valid raw Venom callee uses a plain param as its return PC, retpc_op remains None, so every ret reaches this assertion and aborts compilation. FunctionCallLayout explicitly supports discovering plain return-PC params in raw IR, and the full pipeline runs this inliner before FmpLoweringPass; therefore changing the tests to retpc_param does not eliminate the accepted input path. Resolve the return-PC through the call layout or preserve the previous binding behavior instead of requiring the dedicated opcode here.
Useful? React with 👍 / 👎.
| param_idx = 0 | ||
| for inst in bb.instructions: | ||
| if inst.is_param: | ||
| if inst.opcode == "retpc_param": |
There was a problem hiding this comment.
FunctionCallLayout.return_pc_param() which already resolves retpc properly, could we use that here instead of matching the opcode? Raw written venom with plain 'param" inlines correctly on master.
There was a problem hiding this comment.
Used the FunctionCallLayout.retpc_param_opcode_inst since the retpc_param should exist in the inliner. And added the test to show the problematic case outside of the fuzz test in the other PR
…anPlodky/vyper into fix/venom/pass-disable-testing-fixes
HodanPlodky
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9e10c9eb83
ℹ️ 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".
|
|
||
| layout = FunctionCallLayout(func_copy) | ||
| retpc_inst = layout.retpc_param_opcode_inst | ||
| assert retpc_inst is not None |
There was a problem hiding this comment.
When valid raw Venom invokes a non-returning callee such as a parameterless function ending in stop or revert, its FunctionCallLayout legitimately has no return-PC parameter, yet a single call site selects it for inlining and this assertion aborts compilation. The calling-convention validator explicitly permits callees with no return instructions and expects zero invoke outputs, and the previous inliner could inline this shape because it never required a return-PC binding; only require/remove the parameter when one exists.
Useful? React with 👍 / 👎.
HodanPlodky
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5dc0451895
ℹ️ 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".
| for space in self.invalidate_alias: | ||
| alias_analysis = mem_alias_type_factory(space) | ||
| self.analyses_cache.invalidate_analysis(alias_analysis) |
There was a problem hiding this comment.
When this pass removes only pure dead instructions, such as an unused add, invalidate_alias remains empty and a directly cached MemoryAliasAnalysis survives. However, that analysis retains the DFGAnalysis and BasePtrAnalysis instances invalidated above (DFG invalidation even deletes its internal maps), so a subsequent query such as pointer_uses_may_touch can crash or use stale facts. Preserve the previous unconditional memory-alias invalidation whenever the pass changes IR.
Useful? React with 👍 / 👎.
| # host's own reclaim governs the inlined data. Whether the | ||
| # host publishes is determined solely by the host's own | ||
| # terminators (plain `ret` is callee-save). | ||
| assert retpc_op in inst.operands, retpc_op |
There was a problem hiding this comment.
When a callee declares %retpc = retpc_param but returns through an accepted alias such as %copy = %retpc; ret %copy, this assertion fails because it requires the original variable to occur directly in every return. FunctionCallLayout.param_for_alias and the calling-convention validator explicitly accept assign/phi aliases of the return PC, so valid raw Venom still aborts during inlining even though the dedicated opcode is present. Identify return operands through the layout or rewrite aliases before deleting the parameter.
Useful? React with 👍 / 👎.
| # since they either bump or set only at the end of the function so the | ||
| # removal of the instruction will not effect other instructions. | ||
| # Other write effect should not be removed by this analysis | ||
| assert inst.get_write_effects() == EMPTY or inst.get_write_effects() == FMP |
There was a problem hiding this comment.
When RemoveUnusedVariablesPass is run before LowerDloadPass on valid raw Venom containing an unused %value = dload ..., the instruction is non-volatile and has no uses, but its declared write effect is MEMORY, so this new assertion aborts the pass. The pass previously removed this dead pseudo-load safely, and it has no ordering constraint requiring LowerDloadPass first; either retain that behavior or explicitly handle dload rather than asserting.
Useful? React with 👍 / 👎.
charles-cooper
changed the title
2 participants
What I did
Implementation of the fixes that were shown by the #5219.
venom_to_assembly.py: There was dependency on liveness in theclean_stack_from_cfg_in. For theoffsetandphiinstructions the output could have been in the stack even if was never used.BranchOptimizationPass: In the case that the branch has a literal as a condition (this happens when SCCP is not run) this pass failed on assertion. If thats the case short circuit.RemoveUnusedVariablesPass: Theloadinstructions could be removed by this pass but did no invalidate their respectivememaliasanalysesFunctionInlinerPass:retpc_paramshould not be needed after if function inline. There was an error where because if was still there the label that does no longer exist was still referenced in the code.How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture