compiler: Revamp data alignment #2296

FabioLuporini · 2024-01-16T09:47:28Z

Sorry about the branch name -- it's misleading to an extent

Note that one test is being dropped because I've removed a micro-optimization pass that was just legacy, rarely (if ever) used, and marginally (if any) impactful, all while being a massive PITA to maintain (basically everything revolving around the so called "ROUNDABLE" Property)

fixes #1579

codecov · 2024-01-16T09:51:35Z

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (735eaa5) 86.74% compared to head (e58dc53) 86.69%.

Files	Patch %	Lines
devito/types/basic.py	87.87%	2 Missing and 2 partials ⚠️
devito/types/dense.py	78.94%	1 Missing and 3 partials ⚠️
devito/ir/clusters/algorithms.py	66.66%	1 Missing and 1 partial ⚠️
devito/logger.py	50.00%	1 Missing ⚠️
devito/passes/iet/engine.py	80.00%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2296      +/-   ##
==========================================
- Coverage   86.74%   86.69%   -0.05%     
==========================================
  Files         229      229              
  Lines       42905    42895      -10     
  Branches     7957     7952       -5     
==========================================
- Hits        37217    37188      -29     
- Misses       5012     5024      +12     
- Partials      676      683       +7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

EdCaunt · 2024-01-16T10:47:13Z

devito/arch/archinfo.py

+        Number of items of type `dtype` that can be transferred in a single
+        memory transaction.
+        """
+        assert self.max_mem_trans_nbytes % np.dtype(dtype).itemsize == 0


Would it be worth raising an exception with a message here if this assertion is False? Not sure how easily a user could bump into this, but could imagine it being quite cryptic to a novice user if they did

@EdCaunt
No, because we developers write these classes and upon testing a new one we would be thrown the assert exception right away if the case

@mloubout yes, == 0, the memory transaction nbytes is always a multiple of 2, just like a C base type size

mloubout

Some minor comments but looks good.

mloubout · 2024-01-16T13:48:41Z

devito/arch/archinfo.py

+        Number of items of type `dtype` that can be transferred in a single
+        memory transaction.
+        """
+        assert self.max_mem_trans_nbytes % np.dtype(dtype).itemsize == 0


mloubout · 2024-01-16T13:51:17Z

devito/data/allocators.py

+            padleft, padright = padding
+        except TypeError:
+            padleft, padright = padding, padding
+        if not isinstance(padleft, int) and not isinstance(padright, int):


Note: we need to get rid of all those isinstance(x, int) and switch to our is_integer, so many places break when you end up with a numpy int instead

mloubout · 2024-01-16T13:54:12Z

devito/data/allocators.py

+        #   is copied, you should assign the new shape to the shape attribute
+        #   of the array:
+        array.shape = shape
+        ndarray = array  # At this point it's interpreted as an ndarray


is this line really needed

nah, dropping

mloubout · 2024-01-16T13:55:00Z

devito/operator/operator.py

@@ -624,7 +624,8 @@ def _prepare_arguments(self, autotune=None, **kwargs):

        # Sanity check
        for p in self.parameters:
-            p._arg_check(args, self._dspace[p], am=self._access_modes.get(p))
+            p._arg_check(args, self._dspace[p], am=self._access_modes.get(p),
+                         **kwargs)


Gonna be needed in PRO soon.

I also had a change in OSS while developing this branch, but then I dropped it. I left the **kwargs because I will need it in an upcoming branch.

If you think about it, it makes a lot of sense -- there's a lot of symbolic properties we might want to check they're honored before signing off on the override; properties we aren't checking today

mloubout · 2024-01-16T13:56:17Z

devito/types/array.py

        elif isinstance(padding, tuple) and len(padding) == self.ndim:
-            return tuple((0, i) if isinstance(i, int) else i for i in padding)
+            padding = tuple((0, i) if isinstance(i, int) else i for i in padding)


why right-padding only?

that's all we need for data alignment, I'm yet to find a use case for left-padding, I doubt it exists but 🤷

OK switching to is_integer

mloubout · 2024-01-16T14:04:14Z

devito/types/dense.py

+
+            elif isinstance(space_order, tuple) and len(space_order) == 2:
+                _, space_halo = space_order
+                if not isinstance(space_halo, tuple) or \


can't be just an int to be same left/right?

unfortunately not, think about shifted derivatives and you may have Functions requiring 4 points on the left and 3 on the right

review-notebook-app · 2024-01-17T08:41:40Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

devito/types/dense.py

tests/test_data.py

georgebisbas · 2024-01-17T09:38:05Z

tests/test_data.py

+    def test_w_halo_custom(self):
+        grid = Grid(shape=(4, 4))
+
+        # Custom halo with not enougn entries raises an exception


small typo "enough"

FabioLuporini requested a review from mloubout January 16, 2024 09:47

EdCaunt reviewed Jan 16, 2024

View reviewed changes

mloubout reviewed Jan 16, 2024

View reviewed changes

mloubout added the compiler label Jan 16, 2024

FabioLuporini force-pushed the enhance-halo-setup branch from 15e40e9 to 18aebdc Compare January 17, 2024 08:41

georgebisbas reviewed Jan 17, 2024

View reviewed changes

devito/types/dense.py Show resolved Hide resolved

tests/test_data.py Outdated Show resolved Hide resolved

georgebisbas reviewed Jan 17, 2024

View reviewed changes

FabioLuporini force-pushed the enhance-halo-setup branch from 6957a26 to adc93fc Compare January 25, 2024 08:47

FabioLuporini added 20 commits January 30, 2024 11:21

compiler: Start revamping autopadding

2c21b09

compiler: Extirpate decrepit loop-rounding optimization

c7b601e

compiler: Remove useless runtime checks

060e0de

compiler: Refactor __padding_setup__

e2a1077

compiler: Add platform to DataManager

9233a88

arch: Add Platform.suggested_alignment

c5c058c

compiler: Rework __padding_setup__

1f84038

compiler: Unpick suggested_alignment

2c6439b

misc: Logging's perf_adv -> hint

3ba3403

api: Enable halo customization

feb1f18

compiler: Tweak autopadding

4e53c7a

compiler: Fix autopadding

7268c9c

compiler: Remove useless is_compact attribute

a812bb3

compiler: Fix linearization in presence of autopadding

3cb062d

pep8 happiness

f8f1334

tests: Update expected output after tweaking autopadding

cfa5027

examples: Update expected output

d335eb8

compiler: Use is_integer where possible

6fb992e

compiler: Fix mapify_reduce

7479238

misc: Homogenize docstrings to use single quotes

d85d402

FabioLuporini and others added 3 commits January 30, 2024 11:21

api: Improve halo setup API and docs

733d7bc

examples: Update expected output

3cd6127

compiler: Skip avoid_denormals upon recursive compilation

e58dc53

FabioLuporini force-pushed the enhance-halo-setup branch from 08d4410 to e58dc53 Compare January 30, 2024 11:21

mloubout approved these changes Jan 30, 2024

View reviewed changes

FabioLuporini merged commit 8bb140d into master Jan 30, 2024
31 checks passed

FabioLuporini deleted the enhance-halo-setup branch January 30, 2024 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Revamp data alignment #2296

compiler: Revamp data alignment #2296

FabioLuporini commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading

EdCaunt Jan 16, 2024

mloubout Jan 16, 2024

FabioLuporini Jan 17, 2024

mloubout left a comment

mloubout Jan 16, 2024

mloubout Jan 16, 2024

FabioLuporini Jan 17, 2024

mloubout Jan 16, 2024

FabioLuporini Jan 17, 2024

mloubout Jan 16, 2024

FabioLuporini Jan 17, 2024

mloubout Jan 16, 2024

mloubout Jan 16, 2024

FabioLuporini Jan 17, 2024

FabioLuporini Jan 17, 2024

mloubout Jan 16, 2024

FabioLuporini Jan 17, 2024

review-notebook-app bot commented Jan 17, 2024

georgebisbas Jan 17, 2024

compiler: Revamp data alignment #2296

compiler: Revamp data alignment #2296

Conversation

FabioLuporini commented Jan 16, 2024 • edited Loading

codecov bot commented Jan 16, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

review-notebook-app bot commented Jan 17, 2024

Choose a reason for hiding this comment

FabioLuporini commented Jan 16, 2024 •

edited

Loading

codecov bot commented Jan 16, 2024 •

edited

Loading