-
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: Default to index-mode=int32 #2399
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #2399 +/- ##
==========================================
- Coverage 86.74% 86.72% -0.02%
==========================================
Files 235 235
Lines 44521 44521
Branches 8242 8242
==========================================
- Hits 38619 38611 -8
- Misses 5177 5189 +12
+ Partials 725 721 -4 ☔ 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.
The b2b diff looks ok
devito/__init__.py
Outdated
configuration.add('autopadding', False, [False, True]) | ||
def _preprocess_autopadding(v): | ||
return { | ||
'0': False, |
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.
Does this also need a {False: False}
mapping?
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.
no
>>> False == 0
True
>>> hash(False) == hash(0)
True
devito/ir/support/basic.py
Outdated
@@ -2,6 +2,7 @@ | |||
from functools import cached_property | |||
|
|||
from sympy import S | |||
import sympy |
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 you not import Interval
and Vector
from SymPy on the line above? It would remove this line and tidy up some of the other code
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 could, but don't want to, for clarity. We have our own Interval
and Vector
too
devito/passes/clusters/blocking.py
Outdated
@@ -569,7 +574,8 @@ def next(self, prefix, d, clusters): | |||
class SynthesizeSkewing(Queue): | |||
|
|||
""" | |||
Construct a new sequence of clusters with skewed expressions and iteration spaces. | |||
Construct a new sequence of clusters with skewed expressions and |
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.
Not a comment purely curiosity, but what is "skewing" in this context?
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.
This is some pre-requisite for generating loops for temporal blocking.
You may find more info on this here if interested:
https://link.springer.com/content/pdf/10.1007/BF01407876.pdf
devito/passes/iet/engine.py
Outdated
""" | ||
Proxy for `abstract_object`. | ||
""" | ||
# Expose hidden objects for complete reconstruction | ||
objects = [] |
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.
If order doesn't matter, then deepcopying objects0
or using objects = list(object0)
instead of initialising an empty list would remove the objects.append(i)
line
devito/passes/iet/engine.py
Outdated
AbstractIncrDimension: 3, | ||
BlockDimension: 4, | ||
} | ||
keys = [Bundle, Array, DiscreteFunction, AbstractIncrDimension, BlockDimension] |
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: worth making this a tuple?
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.
note that these comments belong to the parent PR as per this PR's description
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
This is based #2396 hence the huge diff, otherwise it's just ~15 lines change
This is the diff: 7f6762a