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 cpp.reshape_strides(), which currently cannot handle inputs that tries to increase the number of dimensions after the reshape. #1692

Merged
merged 7 commits into from
Nov 8, 2024

Conversation

pratyai
Copy link
Collaborator

@pratyai pratyai commented Oct 17, 2024

Fix the issue with cpp codegen, where it currently cannot handle inputs:

cpp.reshape_strides(Range([(0, 4, 1), (0, 5, 1)]), None, None, [2, 3, 5])

and crashes with an index error.

Also fixes #1690 where RedundantArray was producing a (valid) graph that triggered this.

Copy link
Contributor

@acalotoiu acalotoiu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM -please add the test as discussed

@pratyai pratyai marked this pull request as draft October 23, 2024 16:26
@pratyai pratyai force-pushed the fix-codegen branch 2 times, most recently from 669abe2 to 8022221 Compare October 23, 2024 21:59
@pratyai pratyai marked this pull request as ready for review October 23, 2024 22:00
@pratyai
Copy link
Collaborator Author

pratyai commented Oct 23, 2024

@acalotoiu,

  • The commit ec029bb introduces a test that verifies that there is an exception raised when compiling the transformed graph.
  • There are also additional unit tests that covers various cases (I could add even more if we want to be really through).
  • The fix commit 2960b9f fixes the compiling problem, but now it verifies that even now the transformed graph is not correct.
  • Thanks for insisting on having this case written down. @alexnick83 pointed out earlier that my original input graph (which I started the bug with) had a couple of memlet's ill-defined. So, now I have come up with a slightly different graph to trigger the problem again. Perhaps you would like to see the input graph to confirm that it is indeed a valid SDFG, even if it looks contrived.
    def original_graph_with_redundant_array():
        g = SDFG('prog')
        g.add_array('A', (5, 5), dace.float32)
        g.add_array('b', (1,), dace.float32, transient=True)
        g.add_array('c', (5, 5), dace.float32, transient=True)

        st0 = g.add_state('st0', is_start_block=True)
        st = st0

        # Make a single map that copies A[i, j] to a transient "scalar" b, then copies that out to a transient array
        # c[i, j], then finally back to A[i, j] again.
        A = st.add_access('A')
        en, ex = st.add_map('m0', {'i': '0:1', 'j': '0:1'})
        st.add_memlet_path(A, en, dst_conn='IN_A', memlet=Memlet(expr='A[0:1, 0:1]'))
        b = st.add_access('b')
        st.add_memlet_path(en, b, src_conn='OUT_A', memlet=Memlet(expr='A[i, j] -> b[0]'))
        c = st.add_access('c')
        st.add_memlet_path(b, c, memlet=Memlet(expr='b[0] -> c[i, j]'))
        st.add_memlet_path(c, ex, dst_conn='IN_A', memlet=Memlet(expr='c[i, j] -> A[i, j]'))
        A = st.add_access('A')
        st.add_memlet_path(ex, A, src_conn='OUT_A', memlet=Memlet(expr='A[0:1, 0:1]'))
        st0.fill_scope_connectors()

        g.validate()
        g.compile()
        return g

tests/codegen/targets/cpp_test.py Outdated Show resolved Hide resolved
tests/codegen/targets/cpp_test.py Outdated Show resolved Hide resolved
tests/codegen/targets/cpp_test.py Outdated Show resolved Hide resolved
tests/codegen/targets/cpp_test.py Outdated Show resolved Hide resolved
@pratyai pratyai requested a review from tbennun October 24, 2024 09:09
@pratyai pratyai added no-ci Do not run any CI or actions for this PR and removed no-ci Do not run any CI or actions for this PR labels Oct 24, 2024
@phschaad phschaad added this to the 1.0 milestone Oct 29, 2024
@phschaad phschaad added the bug Something isn't working label Oct 29, 2024
@pratyai pratyai changed the title Fix the issue with cpp codegen, where it currently cannot handle certain valid graphs. Fix cpp.reshape_strides(), which currently cannot handle inputs that tries to increase the number of dimensions after the reshape. Oct 29, 2024
@tbennun tbennun enabled auto-merge November 8, 2024 07:25
@tbennun tbennun added this pull request to the merge queue Nov 8, 2024
Merged via the queue into spcl:main with commit 2911a45 Nov 8, 2024
10 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

RedundantArray can crash during compile() call because the memlet path update is wrong in some cases.
4 participants