-
Notifications
You must be signed in to change notification settings - Fork 129
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
Trivial map elimination init #1353
Trivial map elimination init #1353
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.
LGTM, but maybe check the case I described where the MapEntry has two output edges, one empty and one non-empty.
# Add an edge directly from the previous source connector to the destination | ||
graph.add_edge(path[index - 1].src, path[index - 1].src_conn, edge.dst, edge.dst_conn, edge.data) | ||
else: | ||
write_only_map = True |
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.
It may be unlikely, but one could construct on purpose a MapScope with at least two input edges, one empty and one non-empty. I don't think that this violates semantics. In such a case, write_only_map
would be True,
although the Map would not really be write-only. Perhaps the boolean logic can be rewritten so that the default value is True
and is switched to False
when a non-empty edge is detected. I think this would work for all cases.
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 changed it as suggested and added a testcase in this commit. Though I think there should be an argument if this is wanted. As my code before that commit leads to the situation where the empty edge is added again. One could argue that this is to be expected as it was there before and it is not the job the this transformation to remove these edges. In that case one would probably also have to consider the case if there are any empty memlets as this code assumes there is only one. Which sparks the general question what should happen with them.
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 propose we leave it at the version it is now - assuming it covers current cases.
The EliminateTrivialMap transformation fails to properly connect the content of the map if it is a write only map and the map to be removed is nested inside another map. This is as the current solution uses memlet_path which does not work on the empty memlets going away from the map entry.
This adds a testcase and solution which seems to work for my specific needs but are not polished and might not work in all cases.