compiler: Keep the outer Dimension parallel on a short par-tile - #3021
compiler: Keep the outer Dimension parallel on a short par-tile#3021mloubout wants to merge 1 commit into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #3021 +/- ##
=======================================
Coverage 83.77% 83.77%
=======================================
Files 257 257
Lines 55007 55028 +21
Branches 4708 4710 +2
=======================================
+ Hits 46083 46102 +19
- Misses 8109 8111 +2
Partials 815 815
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:
|
b77ed90 to
173ac59
Compare
FabioLuporini
left a comment
There was a problem hiding this comment.
this change is pretty deep and regardless of the tweaks we're going to make it wants some tests across OSS and PRO (tests that are not simply tuner tests)
0c1f941 to
8962a2a
Compare
FabioLuporini
left a comment
There was a problem hiding this comment.
much better now than the previous iteration, but still a couple of questions
| def __new__(cls, items, default=None, sparse=None, reduce=None): | ||
| # A short entry is padded to the nest it lands on rather than losing its | ||
| # outermost BlockDimensions. Only meaningful on a device | ||
| stretch = False |
There was a problem hiding this comment.
this shouldn't be a class attribute
There was a problem hiding this comment.
It also gets set in separately in the __new__...
There was a problem hiding this comment.
that's why it shouldn't be a class attribute
| """ | ||
| Pad `umt` with its innermost size until it covers `dims`. | ||
|
|
||
| A user-supplied par-tile entry may be shorter than the nest it lands on. |
There was a problem hiding this comment.
essentially this (x loop on the host) would occur for unoptimized kernels, or kernels that are not in normal form (2+1 loops) -- do I get it right?
There was a problem hiding this comment.
however, and regardless, UnboundTuple should keep returning the last entry upon repeated next() calls, see here:
https://github.com/devitocodes/devito/blob/main/devito/tools/data_structures.py#L778
so I'm not sure I understand why any of this would be necessary
There was a problem hiding this comment.
essentially this (x loop on the host) would occur for unoptimized kernels, or kernels that are not in normal form (2+1 loops) -- do I get it right?
yes
8962a2a to
94b8008
Compare
| def __new__(cls, items, default=None, sparse=None, reduce=None): | ||
| # A short entry is padded to the nest it lands on rather than losing its | ||
| # outermost BlockDimensions. Only meaningful on a device | ||
| stretch = False |
There was a problem hiding this comment.
It also gets set in separately in the __new__...
| sparse=oo.pop('par-tile-sparse', None), | ||
| reduce=oo.pop('par-tile-reduce', None)) | ||
| reduce=oo.pop('par-tile-reduce', None), | ||
| stretch=True) |
There was a problem hiding this comment.
A more descriptive name (par-tile-stretch or par-tile-extend) might be good?
| return obj | ||
| elif isinstance(items, bool): | ||
| if not default: | ||
| raise ValueError("Expected `default` value, got None") |
There was a problem hiding this comment.
f"Expected default value, got {default}"?
| stretch=False): | ||
| if not items: | ||
| return UnboundedMultiTuple() | ||
| obj = UnboundedMultiTuple() |
There was a problem hiding this comment.
I know this is pre-existing, but maybe a brief comment explaining what this short-circuit is for wouldn't go amiss
|
|
||
| umt.iter() | ||
|
|
||
| if stretch: |
There was a problem hiding this comment.
Given stretch relies on self.stretch and self.umt, is it worth having this logic within self.stretched?
There was a problem hiding this comment.
I think this comment is now obsolete
| if stretch: | ||
| # Walk this nest's own entry, which repeats its innermost size if | ||
| # it is shorter than the nest, rather than running out of sizes | ||
| return umt.curitem().reset() |
There was a problem hiding this comment.
why the reset() at the end ?
When a multi `par-tile` entry is shorter than the blocked nest, the innermost Dimensions consume the available block sizes and the outermost one runs into a StopIteration, raised by the top-level tuple once the entry is exhausted. It was then dropped into `compact`, which promotes it back to its root Dimension, discarding its BlockDimension. On a device that is a serialization: the Dimension ends up outside the blocked nest, `filter_iterations` rejects it as non-parallel, and the kernel is launched over a 2D grid from a host loop iterating the outer Dimension one slice at a time. Walk this nest's own entry instead, which repeats its innermost size for as long as the nest needs it, so the nest stays fully blocked. Whether to do so is decided when the par-tile is built, where the target is already known, rather than by inspecting a cluster back in the pass. It applies to multi par-tiles on a device only: a single user-supplied par-tile defines the block rank on purpose, and on a host a short entry is the documented way to ask for 2.5D blocking.
94b8008 to
18681a8
Compare
When a multi
par-tileentry is shorter than the blocked nest, the innermost Dimensions consume the available block sizes and the outermost one runs into a StopIteration. It was then dropped intocompact, which promotes it back to its root Dimension, discarding its BlockDimension.Tested in pro