feat[venom]: add multi-output instruction support for stack-based calling convention #4747
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #4747 +/- ##
==========================================
- Coverage 93.23% 93.14% -0.10%
==========================================
Files 137 137
Lines 19452 19627 +175
Branches 3354 3398 +44
==========================================
+ Hits 18136 18281 +145
- Misses 893 914 +21
- Partials 423 432 +9 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
harkal
changed the title
| if inst.output is not None: | ||
| bb_defined.add(inst.output) | ||
| outs = inst.get_outputs() | ||
| if outs: |
| outs = inst.get_outputs() | ||
| if outs: | ||
| for o in outs: | ||
| bb_defined.add(o) # type: ignore |
There was a problem hiding this comment.
maybe add an assertion instead of type: ignore
There was a problem hiding this comment.
actually, o should always be an IRVariable, not sure why the type: ignore is necessary
vyper/venom/basicblock.py
Outdated
|
|
||
| def copy(self) -> IRInstruction: | ||
| ret = IRInstruction(self.opcode, self.operands.copy(), self.output) | ||
| if self._extra_outputs: |
There was a problem hiding this comment.
don't use truthy -- this is wrong in case self._extra_outputs has length 0, the pointer to the empty list will be shared with the original instruction.
i think it might be better to just use copy.copy(self._extra_outputs)
vyper/venom/basicblock.py
Outdated
|
|
||
| inst = IRInstruction("invoke", inst_args, ret) | ||
| inst = IRInstruction("invoke", inst_args, single_output) | ||
| if extra_outputs: |
| # return buffer | ||
| if does_return_data: | ||
| if ENABLE_NEW_CALL_CONV and returns_word: | ||
| # TODO: remove this once we have proper memory allocator |
There was a problem hiding this comment.
why remove these comments?
There was a problem hiding this comment.
comments fell in battle :)
vyper/venom/passes/sccp/sccp.py
Outdated
| self.work_list.append(SSAWorkListItem(target_inst)) | ||
| outputs = inst.get_outputs() | ||
| for out in outputs: | ||
| if out is None: |
vyper/venom/venom_to_assembly.py
Outdated
| if inst.output is not None: | ||
| stack.push(inst.output) | ||
| outs = inst.get_outputs() | ||
| if outs: |
vyper/venom/venom_to_assembly.py
Outdated
| outs = inst.get_outputs() | ||
| if outs: | ||
| # For multi-output instructions, keep outputs on stack even if | ||
| # not immediately live; subsequent instructions may consume them. |
There was a problem hiding this comment.
doesn't this indicate there is an issue with liveness?
There was a problem hiding this comment.
nice work.
couple comments about the approach:
- why split the outputs into output + extra_outputs? seems like it would be cleaner to just have a list
- i think the use of truthy for
outsis going to lead to a lot of trouble since it doesn't distinguish between None and the empty list -- ex. https://github.com/vyperlang/vyper/pull/4747/files#r2476313719
2 participants
What I did
Extent Venom to support multi-output instructions and implemented a multi-output
invokefor functions that return multiple values.How I did it
How to verify it
Commit message
Description for the changelog
Cute Animal Picture