compiler: add rudimentary support for multi-cond buffering#2838
compiler: add rudimentary support for multi-cond buffering#2838
Conversation
c4cd3ac to
cb9731e
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2838 +/- ##
==========================================
+ Coverage 78.95% 78.97% +0.01%
==========================================
Files 248 248
Lines 50877 50901 +24
Branches 4394 4395 +1
==========================================
+ Hits 40170 40197 +27
+ Misses 9908 9905 -3
Partials 799 799 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
cb9731e to
e1a2dfb
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
e1a2dfb to
51a7df1
Compare
EdCaunt
left a comment
There was a problem hiding this comment.
What's the reasoning behind this change?
devito/ir/clusters/algorithms.py
Outdated
| exprs[i] = e.func(*e.args, conditionals=conditionals) | ||
|
|
||
| guards = {d: sympy.And(*v, evaluate=False) for d, v in guards.items()} | ||
| # Combination mode is And by default and Or if all conditions are |
There was a problem hiding this comment.
"Combination mode is And by default. If all conditions are Or then Or combination mode is used." may be less ambiguous
There was a problem hiding this comment.
(nitpicking, with backtips ` around mode)
devito/types/dimension.py
Outdated
| index into arrays. A typical use case is when arrays are accessed | ||
| indirectly via the ``condition`` expression. | ||
| relation: Or/And, default=And | ||
| How this ConditionalDimension will be combined with other ones. |
There was a problem hiding this comment.
Seems slightly unintuitive to specify this here? Surely it would make more sense to specify how ConditionalDimensions are combined at the point of combination?
Perhaps with an API like cdim.and(other) and cdim.or(other) which returns a new ConditionalDimension which combines the conditions of its constituent ConditionalDimensions?
There was a problem hiding this comment.
Surely it would make more sense to specify how ConditionalDimensions are combined at the point of combination?
You cannot so that's not relvant. A Function a defined dimension. It will be combined with any indices and implicit_dims at lowering. Combinining it by hand won't do anything as it will be combined yet again at lowering with And making it useless
There was a problem hiding this comment.
Ah ok, so this is to do with how it's handled during lowering and the combination is not exposed at the API level? It might be worth adding a note to the notebook explaining when you would want to change relation?
The test/MFE |
tests/test_buffering.py
Outdated
| eqs.append(Eq(f.forward, T+1, implicit_dims=ctend)) | ||
|
|
||
| # run operator with buffering | ||
| op = Operator(eqs, opt=('streaming', 'buffering')) |
There was a problem hiding this comment.
you don't need or want "streaming" here
ec47415 to
7707aba
Compare
7707aba to
28fcf9d
Compare
devito/ir/clusters/algorithms.py
Outdated
| exprs[i] = e.func(*e.args, conditionals=conditionals) | ||
|
|
||
| guards = {d: sympy.And(*v, evaluate=False) for d, v in guards.items()} | ||
| # Combination `mode` is And by default and Or if all conditions are |
There was a problem hiding this comment.
ultra-nitpick, can be rephrased/adjusted/fixed, but can go in a different PR
devito/types/dimension.py
Outdated
| relation: Or/And, default=And | ||
| How this ConditionalDimension will be combined with other ones during | ||
| lowering for example combining Function's ConditionalDimension with | ||
| and Equation's implicit_dim. All dimensions within an equation |
There was a problem hiding this comment.
ultra-nitpick: Dimensions (not dimensions)
|
|
||
| __rkwargs__ = DerivedDimension.__rkwargs__ + \ | ||
| ('factor', 'condition', 'indirect') | ||
| ('factor', 'condition', 'indirect', 'relation') |
There was a problem hiding this comment.
not a big fan of the name "relation" as it's a bit vague
5ebeaf8 to
f680b19
Compare
e6428da to
8304fdb
Compare
API up to debate, but need something within these line to allow for the MFE (see test)