Skip to content
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

Merged
merged 9 commits into from
Jan 17, 2024

Conversation

edopao
Copy link
Contributor

@edopao edopao commented Jan 9, 2024

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:
Screenshot 2024-01-09 at 10 52 24

In case of a mapped tasklet, the memlet's closest source/endpoint is a map entry/exit node, like in this other example:
Screenshot 2024-01-09 at 13 15 55

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:
Screenshot 2024-01-12 at 23 10 11

and:
Screenshot 2024-01-12 at 23 15 30

This fix is covered by the following test cases, which would otherwise fail with dace v0.15.1:

tests/cartesian_tests/integration_tests/multi_feature_tests/test_code_generation.py::test_higher_dimensional_fields
tests/cartesian_tests/integration_tests/multi_feature_tests/test_suites.py::TestNon3DFields::test_generation
tests/cartesian_tests/integration_tests/multi_feature_tests/test_suites.py::TestTypedTemporary::test_generation
tests/cartesian_tests/integration_tests/multi_feature_tests/test_suites.py::TestMatrixAssignment::test_generation
tests/cartesian_tests/integration_tests/multi_feature_tests/test_suites.py::TestMatmul::test_generation

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.
@edopao edopao changed the title fix[cartesian]: Fix DaCe array access in tasklet fix[cartesian]: DaCe array access in tasklet Jan 9, 2024
@edopao
Copy link
Contributor Author

edopao commented Jan 9, 2024

cscs-ci run

@edopao edopao marked this pull request as ready for review January 9, 2024 12:40
@edopao edopao requested a review from havogt January 9, 2024 12:40
@edopao
Copy link
Contributor Author

edopao commented Jan 12, 2024

cscs-ci run

@edopao
Copy link
Contributor Author

edopao commented Jan 15, 2024

All problems have been fixed. Coding done, ready for review.

Copy link
Contributor

@FlorianDeconinck FlorianDeconinck left a 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:
"""
Copy link
Contributor

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

Copy link
Contributor

@havogt havogt left a 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.

@edopao edopao merged commit 6283ac9 into GridTools:main Jan 17, 2024
19 checks passed
@edopao edopao deleted the dace-cartesian branch January 17, 2024 14:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants