-
Notifications
You must be signed in to change notification settings - Fork 225
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: Minor tweaks for elastic code gen #2453
base: master
Are you sure you want to change the base?
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2453 +/- ##
==========================================
- Coverage 87.01% 87.00% -0.01%
==========================================
Files 239 239
Lines 44958 44985 +27
Branches 8390 8394 +4
==========================================
+ Hits 39118 39140 +22
- Misses 5108 5113 +5
Partials 732 732 ☔ View full report in Codecov by Sentry. |
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.
How is this affecting elastic?
Code is the same as I understand, do we get faster compilation?
@@ -687,6 +687,10 @@ def __init_finalize__(self, **kwargs): | |||
if not configuration['safe-math']: | |||
self.cflags.append('--use_fast_math') | |||
|
|||
# Optionally print out per-kernel shared memory and register usage | |||
if configuration['profiling'] == 'advanced2': |
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.
cool!
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.
minor comment but looks fine
@@ -490,6 +491,11 @@ def _cache_key(cls, *args, **kwargs): | |||
# From the kwargs | |||
key.update(kwargs) | |||
|
|||
# Any missing __rkwargs__ along with their default values | |||
params = inspect.signature(cls.__init_finalize__).parameters |
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.
Ouch how can we end up in such a weird spot
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.
try the newly added tests without this patch 😬 I don't remember the details, but basically caching bypassed because a different cache key gets computed
@@ -244,6 +246,20 @@ def add(self, expr, make, terms=None): | |||
self[base] = self.extracted[base] = make() | |||
|
|||
|
|||
def subs_if_composite(expr, subs): | |||
""" | |||
Call `expr.subs(subs)` if `subs` contain composite expressions, that is |
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.
Typo: "contains"
@@ -233,7 +233,7 @@ def make_stencil_dimension(expr, _min, _max): | |||
Create a StencilDimension for `expr` with unique name. | |||
""" | |||
n = len(expr.find(StencilDimension)) | |||
return StencilDimension(name='i%d' % n, _min=_min, _max=_max) | |||
return StencilDimension('i%d' % n, _min, _max) |
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.
Nitpick: could these just be min
and max
now?
8eef1a1
to
7f99f6c
Compare
7f99f6c
to
602c448
Compare
No description provided.