-
Notifications
You must be signed in to change notification settings - Fork 229
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
compiler: Fix parlang reductions over >= 4 loops #2417
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2417 +/- ##
==========================================
- Coverage 86.74% 86.70% -0.05%
==========================================
Files 235 235
Lines 44701 44727 +26
Branches 8288 8291 +3
==========================================
+ Hits 38777 38779 +2
- Misses 5195 5221 +26
+ Partials 729 727 -2 ☔ View full report in Codecov by Sentry. |
@@ -664,6 +664,16 @@ def max_mem_trans_size(self, dtype): | |||
assert self.max_mem_trans_nbytes % np.dtype(dtype).itemsize == 0 | |||
return int(self.max_mem_trans_nbytes / np.dtype(dtype).itemsize) | |||
|
|||
def limits(self, compiler=None, language=None): |
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.
Do compiler
and language
get used anywhere? Why couldn't this be a property?
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.
they will in the future yeah, some limits may be compiler and/or language dependent
return cluster | ||
|
||
# If there are more parallel dimensions than the maximum allowed by the | ||
# target platform, we must restrain the number of candidates | ||
max_par_dims = platform.limits()['max-par-dims'] |
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.
why not just platform.max-par-dims
as a property?
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.
because (see comment above) some of these limits may be e.g. programming-model dependent, besides being architecture-dependent
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.
The it's the same as having the compiler store it's complex/f16 dtype it doesn't belong there and should be part of a pass with if/dispatch
devito/arch/archinfo.py
Outdated
language. | ||
""" | ||
return { | ||
'max-par-dims': np.inf, |
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 think np.inf
might be problematic if used for indexing
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.
and indeed one test is failing because of this :D
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.
Nothing to report, apart from fixing the failing tests.
Just OOC, how did this came up as motivation, are people doing any fancy parallelizing stuff for over 4 loops?
norms of time functions with Buffer(1), where the time loop becomes parallel just like x/y/z because you don't have any to/t1/... -- see test |
b22bb1e
to
8c62ae9
Compare
No description provided.