compiler: respect fetch direction #2441
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2441 +/- ##
=======================================
Coverage 87.01% 87.01%
=======================================
Files 236 236
Lines 44900 44899 -1
Branches 8361 8360 -1
=======================================
Hits 39071 39071
+ Misses 5100 5099 -1
Partials 729 729 ☔ View full report in Codecov by Sentry. |
mloubout
force-pushed
the
async-bwd
branch
6 times, most recently
from
ce9e1da to
24f8b31
Compare
FabioLuporini
left a comment
FabioLuporini
left a comment
There was a problem hiding this comment.
We'll need a test for this, ideally here in OSS (test_gpu_common::TestStreaming) or at worst in PRO would be fine too
devito/passes/clusters/asynchrony.py
Outdated
| fetch = e.rhs.indices[d] | ||
| if direction is Forward: | ||
| findex = fetch + 1 | ||
| fshift = 1 if direction is Forward else -1 |
There was a problem hiding this comment.
random thought: we should just set the direction values to -1, 0 (Any), 1
There was a problem hiding this comment.
Yeah good point. 0 might lead to scheduling issues though
devito/passes/clusters/asynchrony.py
Outdated
| if direction is Forward: | ||
| findex = fetch + 1 | ||
| fshift = 1 if direction is Forward else -1 | ||
| if isinstance(fetch, IntDiv) or any(isinstance(a, IntDiv) for a in fetch.args): |
There was a problem hiding this comment.
I don't understand why we need this change.
Searching for IntDiv like that seems fragile to me. Also, either search(fetch, IntDiv) or fetch.find(IntDiv) should be a more compact and robust way of expressing what this line is trying to express
There was a problem hiding this comment.
If you have fetch = IntDiv(time, factor) or some expression of it, then fetch._subs(pd, pd + fshift) will become IntDiv(time+1, factor) which basically does nothing because IntDiv(time, factor) = IntDiv(time+1, factor). Ideally would not want that if but rather find the time dimension in fetch then do a ._sub(t, t+t.spacing) but then need to do an extra {spacing: 1} afterwards too.
THis seemed simpler and less involved and I think covers 99% of the cases
sorry, there is indeed one in PRO. Sorry! |
No description provided.