Fix codegen with data access on inter-state edge #1434
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
After uplift to dace v0.15, one SDFG which was working before started to show compilation errors. The latest DaCe is moving a data access to an inter-state edge. For the data-access, the symbols that define array strides are needed for code generation. The SDFG was validated, before and after the simplify pass, but it did not compile for CPU. When skipping the simplify pass, the compilation did work. The problem has been narrowed down to the scalar-to-symbol promotion, which is moving a data access to an inter-state edge. Then, the method _used_symbols_internal needs to be update to account for data containers, including symbolic shape and strides. This commit contains a unit test to reproduce the issue and verify the proposed fix.
edopao
changed the title
edopao
changed the title
BenWeber42
requested changes
Nov 29, 2023
There was a problem hiding this comment.
Hey, sorry for the delayed review. I briefly talked with Tal, and the changes to DaCe are very good. Thanks a lot for that!
I would like to make some suggestions to the test:
- Move the test to this file:
tests/codegen/codegen_used_symbols_test.py - Try to reduce the test-case. Particularly, build the SDFG directly instead of starting with a different SDFG and then applying transformations until you reach the SDFG that triggers the bug
Regarding (2), I am worried that if the transformations change, then that might nullify the test. We don't really want to test the transformations here (most of the asserts just check that the test should work correctly).
Unit-test reduced to reproduce the origin of this issue. Also, unit-test moved to codegen test file.
|
@BenWeber42 Thank you for the review. Very good comment, I totally agree. I had focused my test more on the trigger of this issue rather than on its cause. I have pushed a commit where I apply the changes you proposed. |
BenWeber42
approved these changes
Dec 4, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
After uplift to dace v0.15, one SDFG which was working before started to show compilation errors. The latest DaCe is moving a data access to an inter-state edge. For the data-access, the symbols that define array strides are needed for code generation. The SDFG was validated, before and after the simplify pass, but it did not compile for CPU. When skipping the simplify pass, the compilation did work. The problem has been narrowed down to the scalar-to-symbol promotion, which is moving a data access to an inter-state edge. Then, the method
_used_symbols_internalneeds to be update to account for data containers, including symbolic shape and strides.This issue was reported in #1433. This PR contains a unit test to reproduce the issue and verify the proposed fix.