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: Enable bufferized stencil.apply #2982

Merged
merged 7 commits into from
Aug 5, 2024

Conversation

PapyChacal
Copy link
Collaborator

I.e., make stencil.apply able to express computations on reference-semantic fields through destination operands, instead of only value-semantic through results.

@PapyChacal PapyChacal added the dialects Changes on the dialects label Aug 5, 2024
@PapyChacal PapyChacal self-assigned this Aug 5, 2024
@PapyChacal PapyChacal requested a review from n-io August 5, 2024 09:18
Copy link

codecov bot commented Aug 5, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 89.85%. Comparing base (5e42115) to head (16cc36e).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2982      +/-   ##
==========================================
- Coverage   89.86%   89.85%   -0.01%     
==========================================
  Files         409      409              
  Lines       51171    51217      +46     
  Branches     7939     7951      +12     
==========================================
+ Hits        45985    46023      +38     
- Misses       3925     3931       +6     
- Partials     1261     1263       +2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@PapyChacal PapyChacal requested a review from georgebisbas August 5, 2024 09:23
@PapyChacal PapyChacal marked this pull request as ready for review August 5, 2024 09:23
if self.dest:
printer.print_list(self.dest, print_destination_operand)
else:
printer.print_list((r.type for r in self.res), printer.print_attribute)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not so keen on this syntax, if I'm understanding correctly that it hides that the bufferized version does not actually have a SSA return value.

Also, can you not have dest+res in the unbufferized mode?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I went with apply only able to be fully value-semantic (having results in res) or reference-semantics (having destinations in dest), never a mix of both

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I could clarify that in a docstring I guess

Copy link
Collaborator

Choose a reason for hiding this comment

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

My point was something else, that dest is printed after the -> (which unhelpfully isn't shown in the 4-line snippet above but comes just before)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Okay, would you happen to have a proposed syntax then? I just went with the easiest to switch to here, and have no strong opinion for that one

Copy link
Collaborator

Choose a reason for hiding this comment

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

To keep it simple, you could have -> ( ... ) for results and dest( .... ) or outs(....) for the destination-style passing.

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could also consider re-using print_assign_argument

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done with outs, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm, thanks for this

if self.dest:
printer.print_list(self.dest, print_destination_operand)
else:
printer.print_list((r.type for r in self.res), printer.print_attribute)
Copy link
Collaborator

Choose a reason for hiding this comment

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

lgtm, thanks for this

@PapyChacal PapyChacal merged commit b98ec7b into main Aug 5, 2024
10 checks passed
@PapyChacal PapyChacal deleted the emilien/stencil-bufferized branch August 5, 2024 14:34
n-io added a commit that referenced this pull request Aug 8, 2024
To match #2982

---------

Co-authored-by: n-io <n-io@users.noreply.github.com>
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.

2 participants