compiler: Faster compilation time#2928
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2928 +/- ##
==========================================
- Coverage 83.35% 82.92% -0.43%
==========================================
Files 248 249 +1
Lines 51734 52402 +668
Branches 4463 4532 +69
==========================================
+ Hits 43122 43456 +334
- Misses 7859 8162 +303
- Partials 753 784 +31
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| def get_visible_devices(): | ||
| device_vars = ( | ||
| 'CUDA_VISIBLE_DEVICES', | ||
| 'NVIDIA_VISIBLE_DEVICES', |
There was a problem hiding this comment.
Is NVIDIA_VISIBLE_DEVICES visible inside the container? I believe it can be combined with CUDA_VISIBLE_DEVICES as well - do we think this is a case we need to cover? Even if not, we should note it in a comment
|
|
||
| @cached_property | ||
| def _writes(self): | ||
| from devito.symbolics.queries import q_routine |
There was a problem hiding this comment.
Implies that maybe the search file is getting a bit tangly and needs to be split up and moved
|
|
||
| @cached_property | ||
| def reads_explicit(self): | ||
| terminals = set(retrieve_accesses(self.rhs, deep=True)) |
There was a problem hiding this comment.
This should already return a set right? I could see potentially wanting an OrderedSet however...
|
|
||
| @cached_property | ||
| def _reads(self): | ||
| return tuple(set(self.reads_explicit) | set(self.reads_conditional)) |
| return tuple(set(self.reads_explicit) | set(self.reads_conditional)) | ||
|
|
||
| @property | ||
| def reads(self): |
There was a problem hiding this comment.
Why is this separate to _reads?
| if all(a is b for a, b in zip(expr.args, args, strict=False)): | ||
| args = tuple(args) | ||
|
|
||
| if type(expr) is tuple: |
There was a problem hiding this comment.
The blocks for tuple and list can be consolidated here I think - it would improve readability
| Like retrieve_terminals, but ensure that if a ComponentAccess is found, | ||
| the ComponentAccess itself is returned, while the wrapped Indexed is discarded. | ||
| """ | ||
| from devito.symbolics.manipulation import uxreplace |
There was a problem hiding this comment.
This implies this file is getting super tangly - there has been other search import oddness in this PR. Maybe something to address sooner rather than later
| else: | ||
| return self._default | ||
|
|
||
| def get(self, key, default=None): |
| body = iet.body._rebuild(body=iet.body.body + (DummyExpr(x, x),)) | ||
| return iet._rebuild(body=body), {} | ||
|
|
||
| monkeypatch.setattr(iet_engine, '_update_args', |
There was a problem hiding this comment.
Is this a Claude test? It seems to love using monkeypatch for tests...
There was a problem hiding this comment.
Leftover? Also the filename is typoed
2 participants
the key ingredients are:
Bonus: a lot of, IMHO, good refactoring. And several new tests.
Compilation time improvements vary between 1.2x and 5x depending on the complexity of the Operator