Conversation
Gas ChangesNo changes detected. Summary
|
![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: 32ba2cecc3
ℹ️ 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".
| retpc_inst = layout.retpc_param_opcode_inst | ||
| assert retpc_inst is not None |
There was a problem hiding this comment.
When the inliner is run on validator-accepted hand-written raw IR such as %retpc = param; ret %retpc, retpc_param_opcode_inst is None and this assertion aborts compilation. FunctionCallLayout.return_pc_param intentionally supports discovering a plain return-PC parameter from the return terminator, so use that semantic property rather than requiring the dedicated frontend opcode.
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 valid raw IR returns through an alias, for example %copy = %retpc; ret %copy, the calling-convention validator resolves that alias through FunctionCallLayout.param_for_alias, but this direct membership assertion rejects it. In addition, the earlier make_nop() leaves the alias instruction referring to an undefined variable, so alias-aware handling is needed rather than requiring every terminator to contain the original parameter variable.
Useful? React with 👍 / 👎.
📊 Bytecode Size Changes (venom)No changes detected. Full bytecode sizes
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7af9f6fd7d
ℹ️ 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.
Validated raw Venom callees may return to a static label without declaring any return-PC parameter, as in ret @retpc (a shape explicitly accepted by the calling-convention tests). When such a callee is selected for inlining, retpc_param_opcode_inst is None, so this assertion aborts compilation even though the previous inliner could rewrite the return normally. Only remove the return-PC definition when the callee actually has one.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 15392bddd3
ℹ️ 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.
A validated raw callee may declare %retpc = retpc_param yet return through a static label, such as ret @exit; the calling-convention validator permits this because the return tail does not resolve to a conflicting parameter. Inlining such a single-call-site callee now aborts at this assertion even though the dedicated parameter can simply be removed and the static return rewritten to the continuation. This remains distinct from the already-reported missing-parameter case because retpc_inst is present here, so merely making its removal conditional would not fix it.
Useful? React with 👍 / 👎.
harkal
left a comment
harkal
left a comment
There was a problem hiding this comment.
codex comments seem legit, all of those are valid raw IR that master inlines fine and now trip the asserts (retpc through a phi and no retpc/no ret too). fixture changes look like they're just there to dodge the assert
wouldn't it be simpler to just keep master's code and not bind retpc to the label?
binding_ops = (*binding_ops[:-1], IRLiteral(0))
your new test passes with that, no fixture changes needed, bytecode identical on everything I tried. the noinline ret fix is good tho, I'd keep that
2 participants
What I did
Implementation of the fix that was shown by the #5219.
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