api: fix handling of multiple conditions for buffering#2850
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2850 +/- ##
==========================================
+ Coverage 82.86% 82.89% +0.02%
==========================================
Files 249 249
Lines 52268 52438 +170
Branches 4499 4539 +40
==========================================
+ Hits 43314 43466 +152
- Misses 8184 8195 +11
- Partials 770 777 +7
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
| return CondNe(*self.args, evaluate=False) | ||
|
|
||
| @property | ||
| def _as_min(self): |
There was a problem hiding this comment.
I would drop this and rather have a singledispatch handler for CondEq where necessary
|
|
||
| def relational_shift(expr, s): | ||
| """ | ||
| Infer shift incurred by the expression. Generally only |
There was a problem hiding this comment.
I could use an example here to quickly visualise what's it trying to do
| expr = uxreplace(expr, {d: IntDiv(index, d.symbolic_factor)}) | ||
|
|
||
| # Merge conditionals when possible. E.g if we have an implicit_dim | ||
| # and there is a dimension with the same parent, we ca merged |
There was a problem hiding this comment.
Dimension
"ca merged"
"their conditions"
you could also make the example a bit more practical
| for d in input_expr.implicit_dims: | ||
| if d not in conditionals: | ||
| continue | ||
| for cd in dict(conditionals): |
| # Replace the ConditionalDimensions in `expr` | ||
| for d, cond in conditionals.items(): | ||
| # Replace dimension with index | ||
| index = d.index |
There was a problem hiding this comment.
you can spare this line
| ispace = IterationSpace(intervals, iterators) | ||
|
|
||
| # Construct the conditionals and replace the ConditionalDimensions in `expr` | ||
| # Construct the conditionals |
There was a problem hiding this comment.
I think we should place this whole block of code, which constructs/lowers the conditionals, into its own separate functions, and a docstring with some examples
ef708e5 to
b997156
Compare
7a1a6aa to
c7786ea
Compare
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
f904760 to
0500469
Compare
| shift = relational_shift(cond, d.parent) | ||
| expr = uxreplace(expr, {d: IntDiv(index, d.symbolic_factor) + shift}) | ||
|
|
||
| # Merge conditionals when possible. E.g if we have an implicit_dim |
There was a problem hiding this comment.
btw this block imho deserves its own function
| if d is not dim: | ||
| continue | ||
|
|
||
| if d in c0.guards and not c0.guards[d].has(Mod): |
There was a problem hiding this comment.
searching for Mod is a bit meh, I'd rather add a special guard to ir/support/guards.py and look for that instead (there's quite a few already in there!)
| _actions_from_update_memcpy(c, d, clusters, actions, sregistry) | ||
| elif d.is_Custom and is_integer(c.ispace[d].size): | ||
| _actions_from_init(c, d, actions) | ||
| _actions_from_init(c, d, clusters, actions) |
|
|
||
|
|
||
| def _actions_from_init(c, d, actions): | ||
| def _actions_from_init(c, d, clusters, actions): |
| "\n", | ||
| " * ``And`` (default): all conditions must hold (mutually exclusive merging).\n", | ||
| " * ``Or``: any one condition is enough for the if-branch to fire.\n", | ||
| " * ``'strict'``: this condition takes precedence over every other condition\n", |
There was a problem hiding this comment.
how about an int, representing priority, instead of strict? it would also make it more natural to generalise it
0500469 to
30790f0
Compare
ab5242a to
5ac1c04
Compare
26dec5c to
5ac8cfb
Compare
5ac8cfb to
e961af6
Compare
No description provided.