-
Notifications
You must be signed in to change notification settings - Fork 49
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
fix[cartesian]: DaCe array access in tasklet #1410
Conversation
Found some incompatible tasklet represention while upgrading to dace v0.15.1. Array access inside tasklet with partial index subset worked in v0.14.1, although not valid. Added view or transient array in case of memlet slice, to ensure that tasklet uses explicit indexes for the full array shape.
cscs-ci run |
cscs-ci run |
All problems have been fixed. Coding done, ready for review. |
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
Test on NASA model
Validation: dace:cpu
: ✔️, dace:gpu
: ✔️
Performance: dace:cpu
: TBD, dace:gpu
: TBD
if dim_size == 1 | ||
] | ||
if len(memlet_data_index) < array_ndims: | ||
""" |
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.
Maybe move this comment at the top of the new for
loop
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 cannot really review the content of the change, but structurally it looks ok. If you are happy with the change @edopao, let's merge.
Found some incompatible tasklet represention while upgrading to dace v0.15.1. Array access inside tasklet with partial index subset worked in v0.14.1, although not valid.
Here is an example of tasklet with partial index subset:
In case of a mapped tasklet, the memlet's closest source/endpoint is a map entry/exit node, like in this other example:
The fix consists of modifying the memlets to pass the full array shape to such tasklet, and use all explicit indices inside the tasklet to access the array. This is the right representation in DaCe SDFG, as discussed with the DaCe developers.
The two tasklet cases shown above, with this fix, become:
and:
This fix is covered by the following test cases, which would otherwise fail with dace v0.15.1: