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

build[next]: rollback gt4py-next to dace 1.0.0, keep cartesian on dace 1.0.1 #1836

Closed
wants to merge 3 commits into from

Conversation

edopao
Copy link
Contributor

@edopao edopao commented Jan 30, 2025

Dace was upgraded to version 1.0.1 to fix a bug in ConstantPropagation in GT4Py cartesian. However, the updated ConstantPropagation transformation breaks code generation for GT4Py next.
This PR does rollback of dace version just for GT4Py next. A deeper investigation of the breaking change will be done later, on dace main branch, because for GT4Py next we plan to pull dace from the git repository.

@philip-paul-mueller
Copy link
Contributor

This is very strange as gt_simplify() is written to explicitly such that this pass is not run by default and as far as I know, we do not call it explicitly anywhere.
So if it is really constant propagation, then it is a violation of ADR18, which clearly states that calling naked DaCe simplify directly is undefined behaviour.
So we have to find this location and remove it.

@philip-paul-mueller
Copy link
Contributor

I think the best way to find this would be to put an assert in SDFG::simplify() and then attach a debugger.
This works because our pipeline does not use this function.

@edopao
Copy link
Contributor Author

edopao commented Jan 30, 2025

I know the exact location. The location is in the PR still to be merged #1818. There I call gt_substitute_compiletime_symbols() when the range start symbols have to be fixed to zero at compile time. Indeed, the only tests that fail are those in dace orchestration, where this option is turned on.

@philip-paul-mueller
Copy link
Contributor

philip-paul-mueller commented Jan 30, 2025

You are absolutely right, I call the transformation there, to handle a case.
But then it is a bug and I myself violate the ADR.

I do not like the current implementation of this function anyway, I opened an issue and think of a solution to avoid that.
There is even a DaCe issue that is related to this problem.

@edopao
Copy link
Contributor Author

edopao commented Jan 30, 2025

dace 1.0.1 is probably not the issue.

@edopao edopao closed this Jan 30, 2025
@edopao edopao deleted the dace-dev branch January 30, 2025 07:24
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