feature[next]: ITIR Inline lift pass extension #1475
Conversation
…enter_deref_lift_vars
…enter_deref_lift_vars
…enter_deref_lift_vars
| @@ -96,6 +98,7 @@ def visit_FunCall(self, node: itir.FunCall, **kwargs): | |||
| eligible_params=eligible_params, | |||
| ) | |||
| # TODO(tehrengruber): propagate let outwards | |||
| # todo: inherit recorded shifts | |||
There was a problem hiding this comment.
Just forgot to remove the todo. The change is one line below.
| flags=InlineLifts.Flag.INLINE_TRIVIAL_DEREF_LIFT | ||
| | InlineLifts.Flag.INLINE_DEREF_LIFT # some tuple exprs found in FVM don't work yet. | ||
| ).visit(ir) | ||
| | InlineLifts.Flag.INLINE_LIFTED_ARGS, | ||
| inline_single_pos_deref_lift_args_only=True, |
There was a problem hiding this comment.
I started with that approach, but it looked rather ugly. The inline_single_pos_deref_lift_args_only=True case is a strict subset of the default case so having both flags doesn't make sense from a user perspective -> already a hint that this is better merely an option to the transformation like it is now. Additionally making two flags means one would need two transform_... functions that then need to dispatch to a third function containing the actual implementation. This is already ugly for the INLINE_DEREF_LIFT, INLINE_TRIVIAL_DEREF_LIFT case and didn't feel right here.
There was a problem hiding this comment.
but it's also not orthogonal to the flags, right? is it a subset of one of the flags?
There was a problem hiding this comment.
Not sure what you mean with orthogonal. It as an option to the INLINE_LIFTED_ARGS flags, either inline all or only the ones that are dereferenced at the center. In that sense it is a subset of that flag.
|
|
||
| import gt4py.eve as eve | ||
| from gt4py.eve import NodeTranslator, traits | ||
| from gt4py.next.iterator import ir | ||
| from gt4py.next.iterator.ir_utils import ir_makers as im | ||
| from gt4py.next.iterator.transforms.inline_lambdas import inline_lambda | ||
| from gt4py.next.iterator.transforms.trace_shifts import TraceShifts, copy_recorded_shifts |
There was a problem hiding this comment.
| from gt4py.next.iterator.transforms.trace_shifts import TraceShifts, copy_recorded_shifts | |
| from gt4py.next.iterator.transforms import trace_shifts |
There was a problem hiding this comment.
Fixed, but good example for a case where I really dislike this because it does not increase readabilty at all:
trace_shifts.TraceShifts.apply(...)
trace_shifts.copy_recorded_shifts(...)
There was a problem hiding this comment.
Maybe the fix would be TraceShifts.apply -> apply. Let's not change that now, but maybe in "workflowify more" project.
havogt
changed the title
| for transformation in self.Flag: | ||
| if self.flags & transformation: | ||
| assert isinstance(transformation.name, str) | ||
| method = getattr(self, f"transform_{transformation.name.lower()}") | ||
| transformed_node = method(new_node, **kwargs) | ||
| # if the transformation returned `None` it did not apply and we continue. | ||
| if transformed_node is not None: | ||
| return transformed_node |
There was a problem hiding this comment.
I am a bit confused by this pattern. The first transformation that matches is applied? Because each transformation matches exactly once?
There was a problem hiding this comment.
Fixed. This should continue transforming.
|
please re-open if still relevant |
Blocked by #1455 (contained in this PR)
Still a draft. While refactoring I found a less complex way for this transformation.