build[next]: switch dace version to main branch from git repo #1835
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
egparedes
reviewed
Jan 29, 2025
2 tasks
This reverts commit 718c619.
The main issue with the change to DaCe main is that now hierarchical control flow blocks are used. Although we do not (yet) generate them simplify might create them. It started in the `DistributedBufferRelocator` which used the `AccessSets` analysis pass. Which now had become unusable, because it merges all accesses in a hierarchical block together. So we switched to the `AccessStates` pass, but now we have to scan the states. However, the main issue was the `is_accessed_downstream()`, because we can no longer simply explore the state machine, we now depend on the `ReachableStates` pass. The downside is that this changed the meaning of the `states_to_ignore`. For now we will life with that but we have to address that in the future.
…und" This reverts commit e2327e8.
The main important changes are: - I added some instance checkes (just to make sure that we do the right thing) that the type checker can never verify. - I also removed most of the accesses to `_pipeline_results` with regards to `StateReachability` as I realized that it was not working anyway. Instead I turned them into TODOs that we must address at some point. However, I think they are not so pressing because our state machines are usually quite small.
There was a problem hiding this comment.
For me it looks good, but I would like to sync with you before, to ask some questions to assess everything.
However, since a lot of changes inside the optimizer were done by me I am not sure if I am the best to review them.
As a side note, most of them are hot fixes and we should fix them, but doing it the proper way would result in several PRs.
edopao
commented
Feb 5, 2025
| prevously_seen_data.add(access_node.data) | ||
|
|
||
| # Now we are collecting all symbols that interstate edges read from. | ||
| if not data.desc(sdfg).transient: |
There was a problem hiding this comment.
I do not understand, these checks are already made outside by the caller.
There was a problem hiding this comment.
They are supposed to handle the case if the function was used on its own, as it should still reprot a correct result.
src/gt4py/next/program_processors/runners/dace/transformations/map_fusion_helper.py
Outdated
Show resolved
Hide resolved
src/gt4py/next/program_processors/runners/dace/transformations/utils.py
Outdated
Show resolved
Hide resolved
…/map_fusion_helper.py Co-authored-by: edopao <[email protected]>
…/utils.py Co-authored-by: edopao <[email protected]>
|
LGTM |
philip-paul-mueller
approved these changes
Feb 5, 2025
| prevously_seen_data.add(access_node.data) | ||
|
|
||
| # Now we are collecting all symbols that interstate edges read from. | ||
| if not data.desc(sdfg).transient: |
There was a problem hiding this comment.
They are supposed to handle the case if the function was used on its own, as it should still reprot a correct result.
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.
Change version of dace dependency in gt4py-next to the main branch of the dace git repository. The version used in gt4py-cartesian remains as before, dace v1.0.1 from the v1/maintenance branch.
This PR includes some changes that were needed to comply with the new API of latest dace:
ConditionalBlockandLoopRegion