feat[venom]: common subexpression elimination pass #4241
Conversation
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| class _Expression: | ||
| first_inst : IRInstruction | ||
| opcode: str | ||
| operands : list[IROperand] |
There was a problem hiding this comment.
looks like this can be IROperand | _Expression
Co-authored-by: Charles Cooper <[email protected]>
…ar fix Co-authored-by: Charles Cooper <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #4241 +/- ##
==========================================
- Coverage 91.40% 90.08% -1.32%
==========================================
Files 112 114 +2
Lines 15927 16172 +245
Branches 2694 2751 +57
==========================================
+ Hits 14558 14569 +11
- Misses 935 1127 +192
- Partials 434 476 +42 ☔ View full report in Codecov by Sentry. |
| # General case | ||
| for self_op, other_op in zip(self.operands, other.operands): | ||
| if not self_op.same(other_op): | ||
| return False |
There was a problem hiding this comment.
i think this is a performance problem. i think the way to get around it is to map all expressions to ids. then we can construct equivalence sets in (roughly) O(n) instead of O(n^2) time.
There was a problem hiding this comment.
i think this approach will make the code cleaner too
|
|
||
| @dataclass | ||
| class _Expression: | ||
| first_inst: IRInstruction |
There was a problem hiding this comment.
maybe just call this inst?
|
|
||
| class AvailableExpressionAnalysis(IRAnalysis): | ||
| expressions: OrderedSet[_Expression] = OrderedSet() | ||
| inst_to_expr: dict[IRInstruction, _Expression] = dict() |
|
|
||
|
|
||
| class AvailableExpressionAnalysis(IRAnalysis): | ||
| expressions: OrderedSet[_Expression] = OrderedSet() |
|
|
||
| if changed: | ||
| for out in bb.cfg_out: | ||
| if out not in worklist: |
There was a problem hiding this comment.
if we require uniqueness in the worklist then we should use OrderedSet. it has a pop() function which is O(1) LIFO pop.
| _MIN_DEPTH = 2 | ||
|
|
||
| UNINTERESTING_OPCODES = ["store", "param", "offset", "phi", "nop"] | ||
| _IDEMPOTENT_INSTRUCTIONS = ["log", "call", "staticcall", "delegatecall", "invoke"] |
There was a problem hiding this comment.
should be NONIDEMPOTENT_INSTRUCTIONS. also i think we should do it the other way. we have to explicitly mark an instruction as idempotent, otherwise it defaults to not-idempotent.
| _MIN_DEPTH = 2 | ||
|
|
||
| UNINTERESTING_OPCODES = ["store", "param", "offset", "phi", "nop"] | ||
| _IDEMPOTENT_INSTRUCTIONS = ["log", "call", "staticcall", "delegatecall", "invoke"] |
There was a problem hiding this comment.
should be NONIDEMPOTENT_INSTRUCTIONS. also i think we should do it the other way. we have to explicitly mark an instruction as idempotent, otherwise it defaults to not-idempotent.
| self.inst_to_available = dict() | ||
| self.bb_outs = dict() | ||
|
|
||
| self.ignore_msize = not self._contains_msize() |
There was a problem hiding this comment.
maybe we should generalize this. basically what this is saying is, "we don't need to care about making memory reads volatile when there is no msize". but it's not that clear from how the code is written. (e.g. maybe there are other write effects which we don't need to preserve if there is no subsequent read).
What I did
Created common subexpression elimination pass
How I did it
Created available expression analysis and common subexpressions elimination pass. The available expression analysis is done with consideration to effects which heavy inspired by multidimensional fencing.
How to verify it
Created tests
Commit message
feat[venom]: common subexpression elimination
Cute Animal Picture