compiler: Revamp lowering of IndexDerivatives #2208

FabioLuporini · 2023-09-18T10:42:11Z

Superseds #2183

No new features added; mostly restructuring, improvements, tweaks, minor fixes

review-notebook-app · 2023-09-18T10:42:15Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

codecov · 2023-09-18T10:47:01Z

Codecov Report

Merging #2208 (c1ebe2f) into master (b6f7308) will decrease coverage by 0.14%.
The diff coverage is 85.45%.

@@            Coverage Diff             @@
##           master    #2208      +/-   ##
==========================================
- Coverage   87.04%   86.91%   -0.14%     
==========================================
  Files         229      229              
  Lines       40955    41545     +590     
  Branches     7498     7660     +162     
==========================================
+ Hits        35648    36107     +459     
- Misses       4695     4816     +121     
- Partials      612      622      +10

Files	Coverage Δ
devito/core/operator.py	`85.16% <100.00%> (ø)`
devito/finite_differences/finite_difference.py	`100.00% <100.00%> (+1.92%)`	⬆️
devito/finite_differences/tools.py	`92.85% <100.00%> (-0.87%)`	⬇️
devito/ir/clusters/algorithms.py	`97.29% <100.00%> (+0.17%)`	⬆️
devito/ir/clusters/visitors.py	`90.08% <100.00%> (-2.55%)`	⬇️
devito/ir/equations/algorithms.py	`100.00% <100.00%> (ø)`
devito/ir/equations/equation.py	`89.47% <100.00%> (ø)`
devito/ir/iet/nodes.py	`92.05% <ø> (ø)`
devito/ir/stree/algorithms.py	`91.55% <100.00%> (ø)`
devito/ir/support/__init__.py	`100.00% <100.00%> (ø)`
... and 48 more

... and 3 files with indirect coverage changes

devito/ir/support/basic.py

georgebisbas

Not finished yet, will look again

georgebisbas · 2023-09-18T11:05:18Z

devito/ir/clusters/algorithms.py

+        try:
+            relations = {tuple(sorted(c.ispace.itdims, key=key))}
+        except ValueError:
+            # See issue #X


2204, fixing

btw; this is not a new issue; it's in fact a long standing issue that now crops up here (as well)

georgebisbas · 2023-09-18T11:14:37Z

devito/ir/clusters/cluster.py

+
+def tailor_properties(properties, ispace):
+    """
+    Create a new Properties object off `properties` that retains all and only


We do not "create", right?

georgebisbas · 2023-09-18T11:32:27Z

devito/ir/support/space.py

@@ -506,7 +536,7 @@ def __getitem__(self, key):
            return super(IntervalGroup, self).__getitem__(key)
        elif isinstance(key, slice):
            retval = super(IntervalGroup, self).__getitem__(key)
-            return IntervalGroup(retval, relations=self.relations)
+            return IntervalGroup(retval, relations=self.relations, mode=self.mode)


empty line as above?

georgebisbas · 2023-09-18T11:33:17Z

devito/ir/support/space.py


    @cached_property
    def nonderived_directions(self):
        return {k: v for k, v in self.directions.items() if not k.is_Derived}
+
+
+null_ispace = IterationSpace([])


devito/finite_differences/finite_difference.py

mloubout · 2023-09-18T14:28:11Z

devito/finite_differences/tools.py

+        """
+        indices = tuple(reversed(self))
+
+        free_dim = StencilDimension(self.free_dim.name,


self.free_dim.transpose

mloubout · 2023-09-18T14:32:51Z

devito/ir/clusters/cluster.py

@@ -308,7 +295,8 @@ def dspace(self):

            # Special case: if the factor of a ConditionalDimension has value 1,
            # then we can safely resort to the parent's Interval
-            intervals = intervals.promote(lambda d: d.is_Conditional and d.factor == 1)
+            key = lambda d: d.is_Conditional and d.factor == 1


Not handled yet but would add d.condition is None for safety as will probably show up to have both factor and condition

mloubout · 2023-09-18T14:36:00Z

devito/ir/clusters/cluster.py

+    for i in properties:
+        for d in as_tuple(i):
+            if d not in ispace.itdims:
+                properties = properties.drop(d)


Why is this needed? It's also slightly cheaper to do that first since everything added above is in ispace.itdims

to remove stuff that would merely represent pollution after e.g. a reconstruction

this doesn't impact functionality; but it makes the objects neater

and yes, ok, I'll move it above

mloubout · 2023-09-18T14:56:16Z

devito/passes/equations/linearity.py

+        if e2 == e1:
+            # No luck, stick to `e` to preserve e.g. the original
+            # coefficient factorization
+            processed.append(e)


Would it be worth to checke1 == e before factorize to skip it if not needed?

it would be wrong. There can be factorization opportunities even if the aggregator spits out the same equations

devito/symbolics/inspection.py

devito/tools/data_structures.py

mloubout · 2023-09-18T15:06:21Z

devito/tools/utils.py

+if py_ge_37:
+    def filter_ordered(elements, key=None):
+        # This method exploits the fact that dictionary keys are unique and ordered
+        # (since Python 3.7). It's concise and often faster for larger lists


How much faster?

significantly, but I don't remember the band, this was one of several compilation speed improvements

Sorry wrote too fast. I mean how "bad" is it with python <3.7 if we just drop the old version

in some benchmarks I've tried it was up to 10% ! But only the bigger ones IIRC (e.g. tti)

For 10% I don't think it's worth keeping both just ditch the numpy one

Turns out I could...

But only as long as we officially discontinue py36.
I had (sort-of) documented this in the comment above:

# This method exploits the fact that dictionary keys are unique and ordered # (since Python 3.7). It's concise and often faster for larger lists

devito/types/dimension.py

FabioLuporini · 2023-09-18T15:51:31Z

I was nearly forgetting.

Bonus:

compilation speed now approx twice as fast

mloubout · 2023-10-02T14:55:48Z

devito/finite_differences/differentiable.py

@@ -693,6 +711,10 @@ def _evaluate(self, **kwargs):
        return expr


+ordering_of_classes.insert(ordering_of_classes.index('Derivative') + 1,


What is this for?

adding comment in upcoming PR

devito/finite_differences/finite_difference.py

mloubout · 2023-10-02T14:57:55Z

devito/finite_differences/tools.py

+        free_dim = self.free_dim.transpose()
+
+        try:
+            expr = self.expr._subs(self.free_dim, -free_dim)


that's not part of self.free_dim.transpose() ?

mloubout · 2023-10-02T15:09:01Z

devito/ir/support/guards.py

+        else:
+            new_args.append(a)
+    if not covered:
+        new_args.append(v)


You have v twice in the case candidates, other = [], [relation, v] so probably if not covered and candidates

the sympy And will automatically simplify it away

mloubout · 2023-10-02T15:11:43Z

devito/ir/support/space.py

        return wrapper

    @_normalize
-    def intersection(self, o):
+    def intersection(self, o, relations, mode):


should relations/mode default to self ? Same below

it's all handled via the _normalize decorator

mloubout · 2023-10-02T15:27:26Z

devito/passes/clusters/aliases.py

+        tot = 0
+        for v in as_mapper(self, lambda i: i.ispace).values():
+            exprs = [i.pivot for i in v]
+            tot += estimate_cost(_cse(exprs, make), True)


_cse is a bit pricey to call here no?

mloubout · 2023-10-02T15:33:14Z

devito/tools/utils.py

+if py_ge_37:
+    def filter_ordered(elements, key=None):
+        # This method exploits the fact that dictionary keys are unique and ordered
+        # (since Python 3.7). It's concise and often faster for larger lists


For 10% I don't think it's worth keeping both just ditch the numpy one

devito/types/dense.py

FabioLuporini added the compiler label Sep 18, 2023

FabioLuporini requested review from mloubout and georgebisbas September 18, 2023 10:42

FabioLuporini mentioned this pull request Sep 18, 2023

compiler: Improve IndexDerivatives lowering #2183

Closed

FabioLuporini commented Sep 18, 2023

View reviewed changes

devito/ir/support/basic.py Show resolved Hide resolved

georgebisbas reviewed Sep 18, 2023

View reviewed changes

mloubout reviewed Sep 18, 2023

View reviewed changes

mloubout reviewed Oct 2, 2023

View reviewed changes

mloubout force-pushed the revamp-opt-derivs-final branch from a96a803 to 7af8fa3 Compare October 2, 2023 17:45

FabioLuporini force-pushed the revamp-opt-derivs-final branch 2 times, most recently from b1fcb66 to 043591c Compare October 9, 2023 07:30

FabioLuporini added 16 commits October 13, 2023 18:10

compiler: Emulate e.find(type) with (faster) search(e, type)

38f50fa

compiler: Add IterationSpace.prefix

3c2ceee

compiler: Use internal repr for IndexSums

ad3287a

compiler: Add Bunch.__repr__

f5be826

compiler: Add ClusterGroup.rebuild

1e88a01

compiler: Add ClusterGroup.properties

95f51b6

comiler: Add minmax_index()

020ad7d

compiler: Add DAG.all_predecessors

ebcee2b

tools: Add DAG.find_paths

866a66c

compiler: Generalize AffineIndexAccessFunction

1db334e

compiler: Fix DefFunction printing

023da18

compiler: aliases.Candidate -> ir.ExprGeometry

34e30f5

compiler: Generalize and enhance ExprGeometry

26dbac2

compiler: Tweak minmax_index

62b341c

compiler: Add and_smart for guards auto-simplification

ed576e5

compiler: Improve Cluster.is_dense

8e89323

FabioLuporini and others added 27 commits October 13, 2023 18:10

compiler: Fix AbstractSymbol.__eq__

16af228

compiler: Relax Dimension._hashable_content

35053d9

compiler: Patch MPINeighborhood reconstruction

dfbe169

compiler: Add TBArray

f142b32

compiler: Speedup codegen

957000b

compiler: Speedup codegen by minimizing relations

1c5ad3d

compiler: Speedup codegen by minimizing relations

019fd4f

compiler: Speedup filter_ordered

7c47a28

compiler: Speedup Dimension.__eq__

2a81f6e

compiler: Speedup DDA by introducing null_ispace

232bda7

compiler: Speedup IntervalGroup.reorder

f069244

compiler: Speedup DDA through lazy TimedAccess

e83fed3

compiler: Speedup DDA by postponing distance calculation

4594ff6

compiler: Speedup IterationSpace.project

c96b266

compiler: Speedup DDA avoiding redundant TimedAccesses

7a3bd90

compiler: Speedup Dependence creation

b9e1056

compiler: Speedup DDA by lazily generating writes

9ef9200

compiler: Refactor factorizer to enable external customization

8b85d75

compiler: Revamp and simplify CIRE's cost model

0c520a8

compiler: Fix unexpansion of tensor objects

503f2ec

compiler: Speedup instantiation of DiscreteFunctions

5d843fb

compiler: Speedup ordering of IndexDerivatives

e72f8e9

compiler: Improve StencilDimension interface

3702873

compiler: Remove post-rebase redundant meth

bfa7375

compiler: Fix IndexDerivative lowering

9580746

compiler: Restrict fusion of FetchUpdates

b4264b1

api: cleanup fd transpose implementation

c1ebe2f

mloubout force-pushed the revamp-opt-derivs-final branch from 9126ec3 to c1ebe2f Compare October 13, 2023 22:10

FabioLuporini merged commit fb32972 into master Oct 16, 2023
32 checks passed

FabioLuporini deleted the revamp-opt-derivs-final branch October 16, 2023 12:04

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

compiler: Revamp lowering of IndexDerivatives #2208

compiler: Revamp lowering of IndexDerivatives #2208

FabioLuporini commented Sep 18, 2023

review-notebook-app bot commented Sep 18, 2023

codecov bot commented Sep 18, 2023 •

edited

Loading

georgebisbas left a comment

georgebisbas Sep 18, 2023

FabioLuporini Sep 19, 2023

FabioLuporini Sep 19, 2023

georgebisbas Sep 18, 2023

FabioLuporini Sep 19, 2023

georgebisbas Sep 18, 2023

georgebisbas Sep 18, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 19, 2023

mloubout Sep 18, 2023

FabioLuporini Oct 16, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 19, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 19, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 18, 2023

mloubout Sep 18, 2023

FabioLuporini Sep 19, 2023

mloubout Oct 2, 2023

FabioLuporini Oct 16, 2023

FabioLuporini commented Sep 18, 2023

mloubout Oct 2, 2023

FabioLuporini Oct 16, 2023

mloubout Oct 2, 2023

mloubout Oct 2, 2023

FabioLuporini Oct 16, 2023

mloubout Oct 2, 2023

FabioLuporini Oct 16, 2023

mloubout Oct 2, 2023

mloubout Oct 2, 2023

		@@ -693,6 +711,10 @@ def _evaluate(self, **kwargs):
		return expr


		ordering_of_classes.insert(ordering_of_classes.index('Derivative') + 1,

compiler: Revamp lowering of IndexDerivatives #2208

compiler: Revamp lowering of IndexDerivatives #2208

Conversation

FabioLuporini commented Sep 18, 2023

review-notebook-app bot commented Sep 18, 2023

codecov bot commented Sep 18, 2023 • edited Loading

Codecov Report

georgebisbas 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

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

FabioLuporini commented Sep 18, 2023

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

codecov bot commented Sep 18, 2023 •

edited

Loading