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

Change strides move assignment outside if #1402

Merged

Conversation

Sajohn-CH
Copy link
Contributor

Adds functions/transformations from my thesis to:

  • Change strides of existing data arrays
  • Move constants assignment outside of if/else construct

Adds fixes for:

  • Codegen getting struct depening on some scope situation
  • Map expansion using hardcoded schedule instead of taking existing onej

Samuel Martin and others added 30 commits August 2, 2023 09:23
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

@acalotoiu
Copy link
Contributor

LGTM

However, first ensure that no tests are failing!

@@ -47,7 +47,7 @@ def apply(self, graph: dace.SDFGState, sdfg: dace.SDFG):
new_maps = [
nodes.Map(current_map.label + '_' + str(param), [param],
subsets.Range([param_range]),
schedule=dtypes.ScheduleType.Sequential)
schedule=current_map.schedule)
Copy link
Collaborator

Choose a reason for hiding this comment

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

will not work for GPU maps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could you elaborate why do you think this would not work in GPU maps? The reason why I put this in is that I had maps expanded which were not sequential and suddenly one of the maps was sequential, which goes against my understanding of what MapExpansion should do. Though when I disable this change some (maybe all, didn't check all) of the failing tests pass again.

@Sajohn-CH
Copy link
Contributor Author

Regarding the failing tests and the change in map expansion. One of the failing tests seems to assume that map expansion sets the expanded map to sequential schedule, as it sets some back to default, if I instead set the innermost map to sequential the test now passes. So I see two possibilities here, either my understanding of map expansion is incorrect or some tests depend on the incorrect behaviour and fixing this would potentially be a new issue.

@alexnick83 alexnick83 merged commit 777083a into spcl:master Nov 8, 2023
9 checks passed
@alexnick83 alexnick83 deleted the change_strides_move_assignment_outside_if branch November 8, 2023 13:41
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.

None yet

4 participants