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

Memlets propagated incorrectly out of nested SDFGs #1738

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

phschaad
Copy link
Collaborator

@phschaad phschaad commented Nov 7, 2024

No description provided.

@tbennun
Copy link
Collaborator

tbennun commented Nov 8, 2024

@phschaad it's not memlet propagation, it's the Python frontend.

The culprit is in newast.py, line 3134-3135:

            if ignore_indices:
                to_squeeze_rng = rng.offset_new(offset, True)

Upon squeezing, the offset of the outer memlet may change the offset of the internal memlet, so you need to offset it back to compensate. The offset for rng currently is the symbol's valid range. However, what you should be offsetting by (rather than the offset of the symbol's first value), is the final offset of the memlets on the outer SDFG. This is not yet computed at this point (as there may be multiple accesses, so the final offset may change).

This was introduced in #876, and the PR is nice because it includes several tests that demonstrate when you would want that offsetting. A correct fix should pass your test and also all of those tests.

As with similar issues, #1696 will fix it by removing this code altogether.

@tbennun tbennun removed this from the 1.0 milestone Nov 8, 2024
@phschaad
Copy link
Collaborator Author

phschaad commented Nov 8, 2024

@tbennun I see, that makes sense. I was expecting this to be a non-issue in 1.x, but I suppose we will need to fix newast.py for 1.0 then.

@tbennun tbennun mentioned this pull request Nov 9, 2024
6 tasks
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.

2 participants