refactor[next]: Refactor ITIR CSE pass #1261
Conversation
Introduced new function `extract_subexpression` that given an expression extract all subexprs and return a new expr with the subexprs replaced.
|
@tehrengruber what's the current status of this PR? Is it still relevant? |
| if not isinstance(node, SymbolTableTrait) and not _is_collectable_expr(node): | ||
| return super().visit(node, **kwargs) | ||
| return super().visit(node, depth=depth + 1, **kwargs) |
There was a problem hiding this comment.
does this make sense? we are visiting the same node, but call the visit of the super class?
There was a problem hiding this comment.
I agree this is not nice. Except for the visit_SymRef case super().visit will dispatch to generic_visit and visit the children where depth + 1 is correct. I'm open for other solutions.
There was a problem hiding this comment.
Would it make sense to call generic_visit directly?
There was a problem hiding this comment.
overload generic_visit and increase depth there.
| else: | ||
| super().visit( | ||
| node, | ||
| depth=depth + 1, |
|
|
||
| def visit(self, node: ir.Node, **kwargs) -> None: # type: ignore[override] # supertype accepts any node, but we want to be more specific here. | ||
| if not isinstance(node, SymbolTableTrait) and not _is_collectable_expr(node): | ||
| return super().visit(node, **kwargs) | ||
|
|
||
| depth = kwargs["depth"] |
There was a problem hiding this comment.
why not pop to avoid the funny pattern in 143?
There was a problem hiding this comment.
(and move into the condition for if_)
There was a problem hiding this comment.
The depth is used also in line 187 so I can not move it into the if_ part. I could also pop it which would make line 143 nicer, but then I would need to manually forward the depth in the other parts. I equally dislike both, pick one if you like to :-)
This PR introduces a new function
extract_subexpressionthat given an ITIR expression extracts all subexprs and returns a new expr with the subexprs replaced by new symbols. The function is extracted from the CSE pass and generalized to be used in the temporary extraction pass.No review required right now, will be reviewed in conjunction with the Temporary pass.