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

feat[next][dace]: Add more debug info to DaCe #1384

Merged
merged 38 commits into from
Jan 23, 2024

Conversation

kotsaloscv
Copy link
Contributor

Description

Add more debug info to DaCe (pass SourceLocation from past/foast to itir, and from itir to the SDFG)

Copy link
Contributor

@petiaccja petiaccja left a comment

Choose a reason for hiding this comment

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

The PR is pretty straightforward (that's a good thing), I left a few comments, but other than that I think there isn't much to change.

However, one thing I'm really missing is tests or any way to ensure that location info is properly transmitted. For example, if someone writes a new iterator IR pass and they don't pass the location, we won't know until somebody tries to look at the generated SDFGs while doing something else. That may be weeks or months down the line and finding the culprit causing the loss of location info will be difficult and annoying.

If you make the location field mandatory in iterator IR, that would do the trick. You could also enforce that location is not None in the SDFG generation, though that's less clear. You could also add a test that verifies if the SDFG has debug info.

src/gt4py/next/ffront/foast_to_itir.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/foast_to_itir.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/foast_to_itir.py Outdated Show resolved Hide resolved
@kotsaloscv
Copy link
Contributor Author

kotsaloscv commented Dec 5, 2023

The PR is pretty straightforward (that's a good thing), I left a few comments, but other than that I think there isn't much to change.

However, one thing I'm really missing is tests or any way to ensure that location info is properly transmitted. For example, if someone writes a new iterator IR pass and they don't pass the location, we won't know until somebody tries to look at the generated SDFGs while doing something else. That may be weeks or months down the line and finding the culprit causing the loss of location info will be difficult and annoying.

If you make the location field mandatory in iterator IR, that would do the trick. You could also enforce that location is not None in the SDFG generation, though that's less clear. You could also add a test that verifies if the SDFG has debug info.

FYI, I have integrated Peter's comments in my PR.
I also experimented on making the SourceLocation a mandatory field in the itir node, but it turns out that we will have to modify the code in countless places. There are so many tests that experiment with itir nodes for which we will have to pass a valid location. I do not think that it worths the investment. As is right now, I find it less intrusive and it is up to us to decide what we want to pass from past/foast to itir.
Regarding the testing of the genereted sdfgs to have a valid bebuginfo field, I have added this check in the code:
https://github.com/kotsaloscv/gt4py/blob/more_debinfo/src/gt4py/next/program_processors/runners/dace_iterator/__init__.py#L222
At least, the developer will always be warned about this field and add it appropriately.

Some tests are failing because they need this modification from DaCe's side: spcl/dace#1469

cc: @havogt @tehrengruber

…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
…tir, and from itir to the SDFG): Preserve Location through Visitors [WIP]
@kotsaloscv
Copy link
Contributor Author

FYI, all the comments have been integrated in the PR and now all the tests are passing.
cc: @tehrengruber @havogt

@kotsaloscv kotsaloscv requested a review from petiaccja January 3, 2024 14:36
…tir, and from itir to the SDFG): Preserve Location through Visitors
src/gt4py/eve/traits.py Outdated Show resolved Hide resolved
src/gt4py/eve/visitors.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/foast_to_itir.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/past_to_itir.py Outdated Show resolved Hide resolved
src/gt4py/next/ffront/past_to_itir.py Outdated Show resolved Hide resolved
…tir, and from itir to the SDFG): Preserve Location through Visitors
…tir, and from itir to the SDFG): Preserve Location through Visitors
…tir, and from itir to the SDFG): Preserve Location through Visitors
…tir, and from itir to the SDFG): Preserve Location through Visitors
…tir, and from itir to the SDFG): Preserve Location through Visitors
…tir, and from itir to the SDFG): Preserve Location through Visitors
Copy link
Contributor

@tehrengruber tehrengruber left a comment

Choose a reason for hiding this comment

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

Changes as discussed in person.

…tir, and from itir to the SDFG): Preserve Location through Visitors
@kotsaloscv kotsaloscv dismissed petiaccja’s stale review January 23, 2024 17:06

already resolved after discussions

@kotsaloscv kotsaloscv merged commit d5cfa7d into GridTools:main Jan 23, 2024
38 checks passed
@kotsaloscv kotsaloscv deleted the more_debinfo branch January 23, 2024 17:13
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.

3 participants