Generalize DSE as compiler passes #1030

FabioLuporini · 2019-12-20T16:12:24Z

This is the last of the restructuring PRs, to go in after #1026 #1027 #1028

codecov · 2020-01-17T08:48:21Z

Codecov Report

Merging #1030 into master will increase coverage by 0.07%.
The diff coverage is 95.28%.

@@            Coverage Diff            @@
##           master   #1030      +/-   ##
=========================================
+ Coverage   90.63%   90.7%   +0.07%     
=========================================
  Files         164     167       +3     
  Lines       23161   23234      +73     
  Branches     3324    3337      +13     
=========================================
+ Hits        20991   21075      +84     
+ Misses       1761    1745      -16     
- Partials      409     414       +5

Impacted Files	Coverage Δ
devito/logger.py	`83.05% <ø> (-2.03%)`	⬇️
devito/exceptions.py	`100% <ø> (ø)`	⬆️
devito/passes/__init__.py	`100% <100%> (ø)`	⬆️
devito/ir/clusters/__init__.py	`100% <100%> (ø)`	⬆️
tests/test_dle.py	`97.27% <100%> (-0.29%)`	⬇️
devito/symbolics/extended_sympy.py	`92.78% <100%> (+6.43%)`	⬆️
devito/ir/support/space.py	`88.23% <100%> (+0.37%)`	⬆️
devito/ir/clusters/cluster.py	`98.51% <100%> (+0.8%)`	⬆️
devito/yask/operator.py	`87.06% <100%> (+1.11%)`	⬆️
devito/ir/stree/algorithms.py	`100% <100%> (ø)`	⬆️
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 95ab69c...5dbf7c5. Read the comment docs.

rhodrin · 2020-01-17T11:17:28Z

devito/ir/support/space.py

+    def _normalize(func):
+        """
+        A simple decorator to normalize the input of operator methods that
+        expect an IntervalGroup as operand.


'... as an operand.'

devito/passes/clusters/aliases.py

rhodrin · 2020-01-17T11:30:04Z

devito/passes/clusters/aliases.py

+
+        # Optimization: no need to retain a SpaceDimension if it does not
+        # induce a flow/anti dependence (below, `i.offsets` captures this, by
+        # telling how much halo will be needed to honour such dependences)


Nitpicking again: '... determining how much halo will be required ...'.

interesting. I thought they were pretty much interchangeable here. Could you elaborate?

rhodrin · 2020-01-17T11:32:39Z

devito/passes/clusters/aliases.py

+            index = writeto.index(dep_inducing[0])
+            writeto = IntervalGroup(writeto[index:])
+        except IndexError:
+            perf_adv("Couldn't optimize some of the detected redundancies")


Super pedantic I know, but "Could not optimize ..." reads better in such situations.

no, you're totally right. Fixing.

rhodrin · 2020-01-17T11:38:01Z

devito/passes/clusters/cse.py

+    # - it sometimes "captures too much", losing factorization opportunities;
+    # - very slow
+    # TODO: a second "sympy" mode will be provided, relying on SymPy's CSE() but
+    # also ensuring some sort of post-processing


... some form of post-processing.

devito/ir/stree/algorithms.py

devito/passes/clusters/rewriters.py

devito/passes/clusters/utils.py

rhodrin · 2020-01-17T13:54:54Z

devito/passes/clusters/utils.py

+                    pass
+                elif not i.function.is_DiscreteFunction:
+                    # It didn't come from the outside and it's not in `mapper`, so
+                    # cannot determine if time-invariant; assume time-varying then


Could you give example of an object for which we can't determine whether it's time-variant or not? Not sure at the moment why we wouldn't be able to determine this.

not off top of my head, but I'm sure I needed this elif.

However, I have plans to drop this whole routine (I want to generalize it to any "Dimension-invariant" -- there's no reason to restrict ourselves to time-invariant).

rhodrin · 2020-01-17T13:58:40Z

tests/test_dse.py

+        f = Function(name='f', grid=grid)
+        f.data_with_halo[:] = 1.
+        u = TimeFunction(name='u', grid=grid, space_order=3)
+        u.data_with_halo[:] = 0.


Is this not/will be 0. by default?

yeah. Just cut-pasting from test_dle. I will drop it.

actually, replaced with 0.5 . I don't like computing on zeros.

FabioLuporini

Pushed updates + yask patch

devito/ir/stree/algorithms.py

FabioLuporini · 2020-01-24T08:45:08Z

devito/ir/support/space.py

+    def _normalize(func):
+        """
+        A simple decorator to normalize the input of operator methods that
+        expect an IntervalGroup as operand.


devito/passes/clusters/aliases.py

FabioLuporini · 2020-01-24T08:47:33Z

devito/passes/clusters/aliases.py

+
+        # Optimization: no need to retain a SpaceDimension if it does not
+        # induce a flow/anti dependence (below, `i.offsets` captures this, by
+        # telling how much halo will be needed to honour such dependences)


interesting. I thought they were pretty much interchangeable here. Could you elaborate?

FabioLuporini · 2020-01-24T08:47:45Z

devito/passes/clusters/aliases.py

+            index = writeto.index(dep_inducing[0])
+            writeto = IntervalGroup(writeto[index:])
+        except IndexError:
+            perf_adv("Couldn't optimize some of the detected redundancies")


no, you're totally right. Fixing.

devito/passes/clusters/rewriters.py

devito/passes/clusters/utils.py

FabioLuporini · 2020-01-24T08:51:34Z

devito/passes/clusters/utils.py

+                    pass
+                elif not i.function.is_DiscreteFunction:
+                    # It didn't come from the outside and it's not in `mapper`, so
+                    # cannot determine if time-invariant; assume time-varying then


not off top of my head, but I'm sure I needed this elif.

However, I have plans to drop this whole routine (I want to generalize it to any "Dimension-invariant" -- there's no reason to restrict ourselves to time-invariant).

FabioLuporini · 2020-01-24T08:52:36Z

tests/test_dse.py

+        f = Function(name='f', grid=grid)
+        f.data_with_halo[:] = 1.
+        u = TimeFunction(name='u', grid=grid, space_order=3)
+        u.data_with_halo[:] = 0.


yeah. Just cut-pasting from test_dle. I will drop it.

FabioLuporini · 2020-01-24T08:56:49Z

tests/test_dse.py

+        f = Function(name='f', grid=grid)
+        f.data_with_halo[:] = 1.
+        u = TimeFunction(name='u', grid=grid, space_order=3)
+        u.data_with_halo[:] = 0.


actually, replaced with 0.5 . I don't like computing on zeros.

mloubout

Small comments (mostly reminders for myself)

mloubout · 2020-01-24T15:26:20Z

devito/operator/operator.py

-                perf("- [Hotspot] %s: %.2f s (%.1f %%)" % (i.lstrip('_'), v, perc))
+        threshold = 20.
+
+        def pprint(timings, indent=''):


nitpicking: pprint is a sympy function, with the amount of sympy we use I'd prefer another name

I'll change it to _emit_timings -- no ambiguities then

mloubout · 2020-01-24T15:27:46Z

devito/passes/clusters/aliases.py

+__all__ = ['cire']
+
+
+MIN_COST_ALIAS = 10


Still can't be user modified easily right?

no, but we can easily turn it into a dict, or better, provide an API. For another PR, I'd say

devito/passes/clusters/aliases.py

mloubout · 2020-01-24T15:30:22Z

devito/passes/clusters/factorization.py

+            except AttributeError:
+                assert w_pows == 0
+
+            # Collect common temporaries (r0, r1, ...)


Note to myself, need test for that part if Iremeber correctly

yes.

Also , we can think of optimizations for factorize. There's ample scope for improvement and testing

mloubout · 2020-01-24T15:33:25Z

devito/symbolics/extended_sympy.py

-        return v
-
-
-def eval_taylor_cos(expr):


note to myself
Even in sigle precision , still callin cos instead of cosf (and sinf/...)

FabioLuporini · 2020-01-27T07:24:10Z

devito/operator/operator.py

-                perf("- [Hotspot] %s: %.2f s (%.1f %%)" % (i.lstrip('_'), v, perc))
+        threshold = 20.
+
+        def pprint(timings, indent=''):


I'll change it to _emit_timings -- no ambiguities then

FabioLuporini · 2020-01-27T07:24:52Z

devito/passes/clusters/aliases.py

+__all__ = ['cire']
+
+
+MIN_COST_ALIAS = 10


no, but we can easily turn it into a dict, or better, provide an API. For another PR, I'd say

FabioLuporini · 2020-01-27T07:26:01Z

devito/passes/clusters/factorization.py

+__all__ = ['factorize']
+
+
+MIN_COST_FACTORIZE = 100


@mloubout same story here.

In the (near) future, I'll add devito/passes/config.py -- a centralised place for all these parameters

FabioLuporini · 2020-01-27T07:26:50Z

devito/passes/clusters/factorization.py

+            except AttributeError:
+                assert w_pows == 0
+
+            # Collect common temporaries (r0, r1, ...)


yes.

Also , we can think of optimizations for factorize. There's ample scope for improvement and testing

This was referenced Jan 10, 2020

Bug for s_o=2 operator. #1060

Closed

Generalize Targets as Operators #1028

Merged

FabioLuporini force-pushed the move-clustering-algos branch from bd374bb to 2971618 Compare January 17, 2020 08:46

rhodrin reviewed Jan 17, 2020

View reviewed changes

devito/passes/clusters/aliases.py Show resolved Hide resolved

rhodrin reviewed Jan 17, 2020

View reviewed changes

georgebisbas reviewed Jan 17, 2020

View reviewed changes

devito/ir/stree/algorithms.py Show resolved Hide resolved

rhodrin reviewed Jan 17, 2020

View reviewed changes

devito/passes/clusters/rewriters.py Show resolved Hide resolved

rhodrin reviewed Jan 17, 2020

View reviewed changes

devito/passes/clusters/utils.py Show resolved Hide resolved

rhodrin reviewed Jan 17, 2020

View reviewed changes

FabioLuporini force-pushed the move-clustering-algos branch from 3256145 to cf9efd4 Compare January 24, 2020 09:22

FabioLuporini commented Jan 24, 2020

View reviewed changes

mloubout reviewed Jan 24, 2020

View reviewed changes

FabioLuporini commented Jan 27, 2020

View reviewed changes

FabioLuporini added 12 commits January 27, 2020 08:33

ir: st_ -> stree_

34a852f

operator: Exploit _specialize_clusters

7ae92ee

ir: Move analyze() inside clusterize()

ce4b3d5

ir: Use set, not list, to track cluster properties

27554aa

passes: Manage properties in Lift

c6f19b8

ir: Add filter_properties

643f88d

ir: Enhance space-related classes

7d014e2

types: Fix dimension._arg_check with non-integer intervals

f0614bf

ir,dse,dle: Hoist minimize_reminders

c58422a

ir: Tweak filter_properties

d46f13c

dse,operator: Rework profiling

2748750

dse: Move everything inside passes/clusters

c4caf15

FabioLuporini added 17 commits January 27, 2020 08:33

passes: Centralize aliases detection

a57f4e5

passes: Centralize cse

0f090d5

passes: Move common routines to utils.py

48be8d1

passes: Centralize factorization

1e3bd95

passes: Centralize extractors

cfe35f1

symbolics,passes: Drop unused polynomials stuff

395d7ba

passes: Merge transformer with rewriter

5f3dcc0

passes: Fix all circular imports

c7770f1

Turn warnings into perf_adv where necessary

ec2c1cc

profiling: Enhance timed_pass

e931b8a

Add timed_pass where suitable

1317d6f

operator: Improve compilation profiling

50f0e03

post-rebase fixups

3de0b59

Fixup docstrings and comments

cf54a82

tests: Replace 0. with 0.something for Function initialization

9f37f06

yask: Override Function._honors_autopadding

064c323

operator: pprint -> _emit_timings

5dbf7c5

FabioLuporini force-pushed the move-clustering-algos branch from cf9efd4 to 5dbf7c5 Compare January 27, 2020 07:39

mloubout merged commit d022ffa into master Jan 27, 2020

mloubout deleted the move-clustering-algos branch January 27, 2020 13:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize DSE as compiler passes #1030

Generalize DSE as compiler passes #1030

FabioLuporini commented Dec 20, 2019

codecov bot commented Jan 17, 2020 •

edited

rhodrin Jan 17, 2020

FabioLuporini Jan 24, 2020

rhodrin Jan 17, 2020

FabioLuporini Jan 24, 2020

rhodrin Jan 17, 2020

FabioLuporini Jan 24, 2020

rhodrin Jan 17, 2020

FabioLuporini Jan 24, 2020

rhodrin Jan 17, 2020

FabioLuporini Jan 24, 2020

rhodrin Jan 17, 2020

FabioLuporini Jan 24, 2020

FabioLuporini Jan 24, 2020

FabioLuporini left a comment

FabioLuporini Jan 24, 2020

FabioLuporini Jan 24, 2020

FabioLuporini Jan 24, 2020

FabioLuporini Jan 24, 2020

FabioLuporini Jan 24, 2020

FabioLuporini Jan 24, 2020

mloubout left a comment

mloubout Jan 24, 2020

FabioLuporini Jan 27, 2020 •

edited

mloubout Jan 24, 2020

FabioLuporini Jan 27, 2020

mloubout Jan 24, 2020

FabioLuporini Jan 27, 2020

mloubout Jan 24, 2020

FabioLuporini Jan 27, 2020 •

edited

FabioLuporini Jan 27, 2020

FabioLuporini Jan 27, 2020

FabioLuporini Jan 27, 2020

		__all__ = ['cire']


		MIN_COST_ALIAS = 10

		__all__ = ['cire']


		MIN_COST_ALIAS = 10

		__all__ = ['factorize']


		MIN_COST_FACTORIZE = 100

Generalize DSE as compiler passes #1030

Generalize DSE as compiler passes #1030

Conversation

FabioLuporini commented Dec 20, 2019

codecov bot commented Jan 17, 2020 • edited

Codecov Report

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 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

mloubout left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FabioLuporini Jan 27, 2020 • edited

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 Jan 27, 2020 • edited

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 Jan 17, 2020 •

edited

FabioLuporini Jan 27, 2020 •

edited

FabioLuporini Jan 27, 2020 •

edited