Better CopyToMap
#1675
Better CopyToMap
#1675
Conversation
If the shapes of thw windows that should be copied is the same, then we do not need the serialization and deserialization stuff. This results in easier SDFGs. It is now also possible to apply the transformation if they do not have matching strides. Why was this present anyway.
It is now a bit cleaner and less verbose. However, I use different indexes for the serialization and not serialization case. This is mainly becuause of the tests.
I never run the special test cases and the updating was not correctly done either.
The original version of the transformation tested if the strides were the same and if so, declined to work. I do not fully understand why this behaviour is there, but this commit renables this behaviour for teh sake of compatibility. However, it adds the possibility to disable this, whcih is done inside bypass tests, which is a very stupid name.
It is now also able to work if one of the subset is `None`.
There is now the new syntax.
| if self.a.desc(sdfg).strides == self.b.desc(sdfg).strides: | ||
| if (not self.ignore_strides) and self.a.desc(sdfg).strides == self.b.desc(sdfg).strides: | ||
| return False | ||
| if self.a.data == self.b.data: |
There was a problem hiding this comment.
Why are we disallowing this? I understand that alarms should light up if we are copying from/to the same container. However, any potential damage is already done if the copy is already on the SDFG. Therefore, I would expect that, in a correct SDFG, to not actually have an overlap among the subsets.
There was a problem hiding this comment.
I am not fully understand your comment about the same container.
Furthermore, I also do not understand why this test of the strides is, for me it does not make any sense.
I kept it because the original transformation had it so I maintained it.
I think that this check is there because of the history of this transformation, as it is used by the code generator.
But honestly I have no idea.
What we could do is change the default of ignore_strides to True and then inside the code generator to False to maintain the old behaviour.
There was a problem hiding this comment.
My question is about checking that we are not copying from/to the same container in line 41. I have the impression that this wasn't there before but maybe I didn't check the diff properly.
There was a problem hiding this comment.
No you checked the diff correctly, I am just stupid.
We can remove it.
| bdesc = bvnode.desc(sdfg) | ||
|
|
||
| edge = state.edges_between(avnode, bvnode)[0] | ||
| src_subset = edge.data.get_src_subset(edge, state) |
There was a problem hiding this comment.
Do you think we may need to call the try_initialize method before attempting to get src/dst subsets here?
There was a problem hiding this comment.
I am actually not sure.
I was hit by this a few time in the past so I am in favour of keeping it.
However, the main reason why this function is used (I think I "discovered" it here) is because even in the original code it is used (see old line 70).
For that reason I decided to keep it.
But if you think we should remove that I will not object.
There was a problem hiding this comment.
I mean, should we also add a call to the method that tries to initialize the edge/memlet to ensure that the src/dst subsets are not None when they are not supposed to.
github-merge-queue
bot
removed this pull request from the merge queue due to no response for status checks
By default the transformation uses some linearization followed by a delinearization approach, while this is needed to copy certain shapes, it is unnecessarily complicated for memlets such as
a[0:10, 20:30] -> 40:50, 60:70.This PR adds special cases where the source and destination subset has the same size and transforms it to a simple copy.
It also supports the case where some dimensions are one, i.e. memlets such as
a[0:10, 0:10] -> 0:10, 1, 0:20.For all cases tests were added.
Most importantly the transformation now applies if the strides are the same.
Before this case was blocked.
This PR helps to avoid errors that are related to Issue#1674, but it is not a fix or a solution to it.