-
Notifications
You must be signed in to change notification settings - Fork 49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feature[next]: Non-tree-size-increasing collapse tuple on ifs #1762
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Need more documentation of the new trafo or additionally a more detailed explanation on how it works.
@@ -185,6 +229,8 @@ def transform(self, node: ir.Node, **kwargs) -> Optional[ir.Node]: | |||
method = getattr(self, f"transform_{transformation.name.lower()}") | |||
result = method(node, **kwargs) | |||
if result is not None: | |||
assert result is not node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why is this assert useful?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In case one (erronously) returns the same node again instead of None
to signify no change, fp_transform
runs into an endless loop. This is a cheap way to prevent this case.
self, node: ir.FunCall, **kwargs | ||
) -> Optional[ir.Node]: | ||
if not cpm.is_call_to(node, "if_"): | ||
for i, arg in enumerate(node.args): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here we iterate over any functions args or do we know more?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No we don't know more. We need to build and simplify the continuation first to know if we want to transform. It might be possible to handle multiple ifs, but the code would become more complex and error prone, and there is no benefit. It is just important to remember that the entire node is split into the continuation and the first matching if
argument.
Contrary to the regular inference, this method does not descend into already typed sub-nodes | ||
and can be used as a lightweight way to restore type information during a pass. | ||
|
||
Note that this function is stateful, which is usually desired, and more performant. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where is the statefulness?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reformulated:
Note that this function alters the input node, which is usually desired, and more performant.
tests/next_tests/unit_tests/iterator_tests/transforms_tests/test_collapse_tuple.py
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only have a few proposals to improve readability.
Removing the tuple expressions across
if_
calls on ITIR has been a pain point in the past. While thePROPAGATE_TO_IF_ON_TUPLES
option of theCollapseTuplePass
works very reliably, the resulting increase in the tree size has been prohibitive. With the refactoring to GTIR this problem became much less pronounced, as we could restrict the propagation to field-level, i.e., outside of stencils, but the tree still grew exponentially in the number of references to boolean arguments used insideif_
conditions. This PR adds an additional optionPROPAGATE_TO_IF_ON_TUPLES_CPS
to theCollapseTuplePass
, which is similar to the existingPROPAGATE_TO_IF_ON_TUPLES
, but propagates in the opposite direction, i.e. into the tree. This allows removal of tuple expressions acrossif_
calls without increasing the size of the tree. This is particularly important forif
statements in the frontend, where outwards propagation can have devastating effects on the tree size, without any gained optimization potential. For exampleis problematic, since
PROPAGATE_TO_IF_ON_TUPLES
would propagate, and hence duplicate,complex_lambda
three times, while we only want to get rid of the tuple expressions inside of theif_
s. Note that this transformation is not mutually exclusive toPROPAGATE_TO_IF_ON_TUPLES
.