-
Notifications
You must be signed in to change notification settings - Fork 71
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
(minor): allow csl_stencil.access to operate on own data #2777
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2777 +/- ##
==========================================
- Coverage 89.79% 89.79% -0.01%
==========================================
Files 376 376
Lines 48079 48099 +20
Branches 7370 7375 +5
==========================================
+ Hits 43172 43189 +17
- Misses 3757 3759 +2
- Partials 1150 1151 +1 ☔ View full report in Codecov by Sentry. |
${type(op) == memref.MemRefType} - for accesses to data prefetched from neighbors | ||
${type(op) == stencil.Temp} - for accesses to own data |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a bit concerned about the reckless mixing of value and reference semantics here. Not sure what to think of it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a valid concern, and I suppose is owed to accesses to neighbour data operating on prefetched data, having effectively gone through one more round of lowering as compared to accesses to own data.
The plan was to not touch accesses to own data and leave them as stencil.access
(without lowering to csl_stencil.access
). What this means in the code is that there already is a mixing of value and reference semantics, just on different ops.
The question then partly becomes, how to do this cleanly. I see three ways:
- allow
stencil.access
to have parents other thanstencil.apply
- allow mixing of value semantics (own data) and reference semantics (neighbour data), as proposed here
- separate the ops into something along the lines of
csl_stencil.access_own_data
andcsl_stencil.access_neighbor_data
- this would work but not be my personal preference because of potential confusion around theget_accesses
function. The ops would basically be the same and there may not be any additional gain in this split
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@PapyChacal what would you suggest?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, if in doubt, press merge. Having slept over this for a night, I think it's a symptom of us not having figured out quite how/when bufferisation kicks in. Maybe this isn't a problem we need to solve, maybe it is.
Let's see where this PR takes us instead of sweating too much with the semantics right now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🚀
Follow-up from #2766
The original plan was to lower
stencil.access
tocsl_stencil.access
if and only if the access is to neighbour data (i.e. prefetched), while accesses to own data were supposed to remainstencil.access
without being lowered.This runs into the very practical problem, that
stencil.access
can only exist within astencil.apply
op - which we also want to lower, but this cannot be done withstencil.access
ops.