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: (stencil) Add access pattern analysis #1409

Merged
merged 5 commits into from Aug 7, 2023

Conversation

AntonLydike
Copy link
Collaborator

This adds .get_accesses() to the stencil apply operation, which returns an AccessPattern for each stencil field.

Each access pattern contains a list of accesses, and offers some helpers like is_diagonal, get_diagonals, etc.

@AntonLydike AntonLydike added the dialects Changes on the dialects label Aug 3, 2023
@AntonLydike AntonLydike self-assigned this Aug 3, 2023
@codecov
Copy link

codecov bot commented Aug 3, 2023

Codecov Report

Patch coverage: 80.24% and project coverage change: -0.04% ⚠️

Comparison is base (04809ea) 90.03% compared to head (5cd9aa8) 90.00%.
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1409      +/-   ##
==========================================
- Coverage   90.03%   90.00%   -0.04%     
==========================================
  Files         211      211              
  Lines       26265    26346      +81     
  Branches     3902     3924      +22     
==========================================
+ Hits        23648    23713      +65     
- Misses       2004     2014      +10     
- Partials      613      619       +6     
Files Changed Coverage Δ
xdsl/dialects/stencil.py 92.63% <73.33%> (-3.62%) ⬇️
tests/dialects/test_stencil.py 100.00% <100.00%> (ø)

... and 1 file with indirect coverage changes

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

@@ -631,3 +632,31 @@ def test_buffer():
assert isinstance(buffer, BufferOp)
assert buffer.temp == temp
assert buffer.res.type == res_type


def test_access_patterns():
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice

Copy link
Collaborator

@PapyChacal PapyChacal left a comment

Choose a reason for hiding this comment

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

Nice! Only minor nitpicks, nothing to block for if using it is urgent 🙂

Comment on lines 796 to 799
for ax in self.accesses:
for axis in range(n):
lefts[axis] = min(ax[axis], lefts[axis])
rights[axis] = max(ax[axis], rights[axis])
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could reuse extent_in_axis

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 know :( but then it would be O(n^2) instead of O(n)...

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't see it?
If you swap the loops, the inner one is exactly extent_in_axis, and the total operations are the same, or am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, because one of those is element-wise max?

I would prefer readability before this complexity becomes a problem, I'm not sure we'll ever have asymptotically big numbers of accesses.

But it's nitpicking 🙂

Comment on lines 793 to 800
def halos(self) -> tuple[tuple[int, int], ...]:
n = self.dims
lefts, rights = [0] * n, [0] * n
for ax in self.accesses:
for axis in range(n):
lefts[axis] = min(ax[axis], lefts[axis])
rights[axis] = max(ax[axis], rights[axis])
return tuple(zip(lefts, rights))
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd say this one could use a docstring

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh definitely^^

xdsl/dialects/stencil.py Outdated Show resolved Hide resolved
@AntonLydike AntonLydike merged commit cc136de into main Aug 7, 2023
10 checks passed
@AntonLydike AntonLydike deleted the anton/add-stencil-access-patterns branch August 7, 2023 13:40
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