refact[next][dace]: split handling of let-statement lambdas from stencil body #1781
Conversation
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_builtin_translators.py
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py
Outdated
Show resolved
Hide resolved
| @@ -1309,3 +1320,38 @@ def visit_SymRef(self, node: gtir.SymRef) -> IteratorExpr | MemletExpr | SymbolE | |||
| # if not in the lambda symbol map, this must be a symref to a builtin function | |||
| assert param in gtir_python_codegen.MATH_BUILTINS_MAPPING | |||
| return SymbolExpr(param, dace.string) | |||
|
|
|||
| def apply( | |||
There was a problem hiding this comment.
I don't remember that there is a call to apply().
As far as I can tell apply is only called in this function, so it is a recursive function.
However, it does some highly non trivial pre and post processing.
So I think that:
- This function needs a better name,
applyis just too generic. - This function needs a doc string.
- This function needs comment that explain why the pre and post processing is needed and how it is done.
There was a problem hiding this comment.
I would like to keep the name apply for the entry point of this visitor class, since it is consistent with other visitor classes in GT4Py. However, I agree on the rest so I will write some documentation.
There was a problem hiding this comment.
I still think that apply() is the wrong name. If it is for compatibility, then why was it there before?
_vistit_let() should be the much better name.
There was a problem hiding this comment.
I have now made a code change in this direction.
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py
Outdated
Show resolved
Hide resolved
| @@ -1309,3 +1320,38 @@ def visit_SymRef(self, node: gtir.SymRef) -> IteratorExpr | MemletExpr | SymbolE | |||
| # if not in the lambda symbol map, this must be a symref to a builtin function | |||
| assert param in gtir_python_codegen.MATH_BUILTINS_MAPPING | |||
| return SymbolExpr(param, dace.string) | |||
|
|
|||
| def apply( | |||
There was a problem hiding this comment.
I still think that apply() is the wrong name. If it is for compatibility, then why was it there before?
_vistit_let() should be the much better name.
| prev_symbol_map = self.symbol_map | ||
| self.symbol_map = self.symbol_map.copy() |
There was a problem hiding this comment.
| prev_symbol_map = self.symbol_map | |
| self.symbol_map = self.symbol_map.copy() | |
| prev_symbol_map = self.symbol_map.copy() |
There was a problem hiding this comment.
I was thinking to save the self.symbol_map object as prev_symbol_map, just to be on the safe side.
There was a problem hiding this comment.
In my view this is indicating that you have some big problems.
There was a problem hiding this comment.
I see what you mean, side effects. Luckily I do not have such problems: I can do the change you propose.
However, in my opinion, I still prefer the original version where I added/removed items from always the same dictionary object, without copying it.
src/gt4py/next/program_processors/runners/dace_fieldview/gtir_dataflow.py
Outdated
Show resolved
Hide resolved
That is true. I will make another change in this direction. |
This is a refactoring of the code to lower lambda nodes: it splits the lowering of let-statements from the lowering of stencil expressions.