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

dialects: extend the stencil lowerings #1028

Merged
merged 10 commits into from May 31, 2023

Conversation

PapyChacal
Copy link
Collaborator

This adds support for returning stencil.field from a func.func, and using stencil.fields as scf.for carried variables.

@PapyChacal PapyChacal self-assigned this May 30, 2023
@PapyChacal PapyChacal added the dialects Changes on the dialects label May 30, 2023
Base automatically changed from emilien/stencil-types-bounds to main May 30, 2023 19:10
@codecov
Copy link

codecov bot commented May 30, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +0.03 🎉

Comparison is base (671d571) 86.89% compared to head (e363183) 86.92%.

❗ Current head e363183 differs from pull request most recent head 8b4c03a. Consider uploading reports for the commit 8b4c03a to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1028      +/-   ##
==========================================
+ Coverage   86.89%   86.92%   +0.03%     
==========================================
  Files         125      125              
  Lines       19339    19355      +16     
  Branches     2933     2940       +7     
==========================================
+ Hits        16804    16825      +21     
+ Misses       2033     2030       -3     
+ Partials      502      500       -2     
Impacted Files Coverage Δ
xdsl/dialects/experimental/stencil.py 95.45% <100.00%> (+0.01%) ⬆️
.../transforms/experimental/ConvertStencilToLLMLIR.py 90.55% <100.00%> (+0.64%) ⬆️

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

Copy link
Contributor

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

I had a first look at the implementation and this looks very good!
I wish to have a deeper look as soon as I get more time.

Do you think it makes sense to split this PR in two PRs, one for implementing changes in return_target_analysis (due to recent modification in stencil types) and another small one for adding support related to scf.for variable propagation?

Copy link
Collaborator

@AntonLydike AntonLydike left a comment

Choose a reason for hiding this comment

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

🚀 I think it's pretty good already, some minor nitpicks and docstrings would be nice tho!

xdsl/transforms/experimental/ConvertStencilToLLMLIR.py Outdated Show resolved Hide resolved
xdsl/transforms/experimental/ConvertStencilToLLMLIR.py Outdated Show resolved Hide resolved
xdsl/transforms/experimental/ConvertStencilToLLMLIR.py Outdated Show resolved Hide resolved
@PapyChacal
Copy link
Collaborator Author

I had a first look at the implementation and this looks very good!
I wish to have a deeper look as soon as I get more time.

Do you think it makes sense to split this PR in two PRs, one for implementing changes in return_target_analysis (due to recent modification in stencil types) and another small one for adding support related to scf.for variable propagation?

I think you had a deep enough look to raise this good point 😉
To answer, as a best practice, it would be nice, but we're back in iteration mode, so I wouldn't bother unless you really want it, as a fellow stencil owner 🙂

Copy link
Contributor

@meshtag meshtag left a comment

Choose a reason for hiding this comment

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

To answer, as a best practice, it would be nice, but we're back in iteration mode, so I wouldn't bother unless you really want it, as a fellow stencil owner slightly_smiling_face

No strict requirement from my side

LGTM aside from some minor comments and a possibly trivial question ;)

xdsl/transforms/experimental/ConvertStencilToLLMLIR.py Outdated Show resolved Hide resolved
xdsl/transforms/experimental/ConvertStencilToLLMLIR.py Outdated Show resolved Hide resolved
xdsl/transforms/experimental/ConvertStencilToLLMLIR.py Outdated Show resolved Hide resolved
@PapyChacal PapyChacal force-pushed the emilien/stencil-lowering-iterate branch from 6dfdc87 to 3807ab3 Compare May 31, 2023 13:57
@PapyChacal PapyChacal force-pushed the emilien/stencil-lowering-iterate branch from 00e8dcf to 4aa75ac Compare May 31, 2023 14:23
@PapyChacal PapyChacal force-pushed the emilien/stencil-lowering-iterate branch from e363183 to 8b4c03a Compare May 31, 2023 14:32
@PapyChacal PapyChacal merged commit e4bdb58 into main May 31, 2023
11 checks passed
@PapyChacal PapyChacal deleted the emilien/stencil-lowering-iterate branch May 31, 2023 14:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
dialects Changes on the dialects
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants