feat[next-dace]: Updated MoveDataflowIntoIfBody
#2460
Conversation
src/gt4py/next/program_processors/runners/dace/transformations/auto_optimize.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/transformations/move_dataflow_into_if_body.py
Outdated
Show resolved
Hide resolved
| for oedge in state.out_edges(reloc_node) | ||
| if oedge.dst is not if_block | ||
| ): | ||
| nodes_proposed_for_reloc.discard(reloc_node) |
There was a problem hiding this comment.
Why discard() and not remove()? remove() is more strict because it also checks that it exists, right? and the node has to exists in nodes_proposed_for_reloc
There was a problem hiding this comment.
This is an interesting point.
If you look at line 680, you see that sometimes multiple nodes are removed in one go, this means because you make a copy you might have already removed that node in a previous iteration, thus the node might be no longer inside.
However, your comment made me think and I now changed it, such that at the beginning, it is checked if the node is still there.
Which is probably nicer and a bit faster, because you perform an early exit.
| return branch_nodes | ||
|
|
||
| else: | ||
| # Only some of the incoming nodes will be moved into the `if` |
There was a problem hiding this comment.
The first part of the comment is misleading, in my opinion. I would write:
| # Only some of the incoming nodes will be moved into the `if` | |
| # Only some of the incoming nodes could be moved into the `if` |
There was a problem hiding this comment.
You are right your suggestion is better.
|
|
||
| else: | ||
| # Only some of the incoming nodes will be moved into the `if` | ||
| # body. This is legal only if the not moved nodes are |
There was a problem hiding this comment.
and below:
| # body. This is legal only if the not moved nodes are | |
| # body. It would be legal to relocate the node if all incoming nodes were access nodes, this is why access nodes are not included in 'incoming_nodes'. | |
| # In this case, we cannot relocate the node, so we discard also those 'incoming_nodes' that were candidates for relocation. |
There was a problem hiding this comment.
I kind of like this suggestion, but it is partially a repetition of the comments about input_nodes.
However, I have renamed input_nodes (which is a bad name) to non_mappable_input_nodes which is a much more accurate then the old one and have updated the description by incorporating your suggestions.
I also have refactored it a bit it should be now simpler.
...essor_tests/runners_tests/dace_tests/transformation_tests/test_move_dataflow_into_if_body.py
Outdated
Show resolved
Hide resolved
...essor_tests/runners_tests/dace_tests/transformation_tests/test_move_dataflow_into_if_body.py
Outdated
Show resolved
Hide resolved
edopao
changed the title
MoveDataflowIntoIfBodyMoveDataflowIntoIfBody
|
No degradation is observed. |
|
I did an experiment with MuPhys' Graupel program, with the following results: The results:
It seems that the two feature combined lead to slightly worse performance, but this could also be an artifact. |
This is a 3% difference which might be just due to the variability of the runtime. We can check though if there's a difference in the number of nodes of the SDFG in total or check a ConditionalBlock to see if we get the expected behavior. |
|
@iomaganaris It is probably nothing, but if you have the setup do do a quick check this would be very appreciated. |
3 participants
Updates the way how the set of data that can be moved into an
ifblock is computed. While not strictly adding new capabilities this PR refines the condition to properly handle some condition found in MuPhys.