compiler: Add various performance optimization variants #2446

FabioLuporini · 2024-08-21T10:04:24Z

No description provided.

codecov · 2024-08-21T10:24:28Z

Codecov Report

Attention: Patch coverage is 97.20497% with 9 lines in your changes missing coverage. Please review.

Project coverage is 87.09%. Comparing base (40b081e) to head (b1a10ee).
Report is 17 commits behind head on master.

Files with missing lines	Patch %	Lines
devito/core/operator.py	66.66%	1 Missing and 1 partial ⚠️
devito/ir/support/utils.py	50.00%	2 Missing ⚠️
devito/passes/clusters/cse.py	98.58%	1 Missing and 1 partial ⚠️
devito/passes/clusters/factorization.py	93.54%	2 Missing ⚠️
devito/ir/clusters/visitors.py	66.66%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #2446      +/-   ##
==========================================
+ Coverage   87.04%   87.09%   +0.04%     
==========================================
  Files         237      238       +1     
  Lines       44998    45106     +108     
  Branches     8372     8405      +33     
==========================================
+ Hits        39167    39283     +116     
+ Misses       5102     5091      -11     
- Partials      729      732       +3

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

EdCaunt · 2024-08-21T11:20:39Z

devito/passes/clusters/cse.py

+        * 'cse-algo': str. The CSE algorithm to apply. Accepted: ['basic',
+          'smartsort', 'advanced']. Default is 'basic'.
    """
+    min_cost = options['cse-min-cost']


Are defaults set on these somewhere else?

core/operator.py core/cpu.py etc like all other options

EdCaunt · 2024-08-21T11:24:52Z

devito/passes/clusters/cse.py

+    -----
+    We're not using SymPy's CSE for three reasons:
+
+        * It also captures array index access functions (e.g., i+1 in A[i+1]);


In future, we will want to capture these for things like x+1-ltkn and INT(abs(x)) etc, but we probably want some kind of filter specifying a minimum verbosity

We will also probably want a unique kind of variable name to improve readability. Something like i0 rather than r0

devito/symbolics/manipulation.py

mloubout

Bunch of comments, looks nice.

devito/passes/clusters/cse.py

mloubout · 2024-08-21T12:13:42Z

devito/passes/clusters/cse.py

+        return subs
+
+
+def catch(exprs, mode):


retrieve_cse to match or "search" names

I can't use search because it'd be an alias of symbolics.search, and retrieve_cse is misleading given what "cse" means and in particular given that we're just looking for potential common subexpressions (that is, candidates)

I think catch (for Candidates) is fine

mloubout · 2024-08-21T12:26:32Z

devito/passes/clusters/cse.py

+
+    mapper[Candidate(expr)].append(expr)
+
+    for n in range(2, len(expr.args)):


This is actually fairly "limited" because it ignores the fact Add/Mul are commutative. We would catch a lot more cse if we take commutation in consideration and take all sub-args of length at least two.

So you can do

for terms in sub_args(expr.args):

with

def sub_args(args): nargs = [] for r in range(2, len(args) + 1): nargs.extend(itertools.combinations(args, r)) return nargs

Wich for example for a*b*c*d would capture all a*b, a*c, a*d, b*d, ...

yes, this is all carefully explained in the docstring... it's an expensive algorithm and deliberately we're implementing the heuristic I chose (relying already on the canonical ordering of the arguments etc etc)

devito/passes/clusters/factorization.py

mloubout · 2024-08-21T12:30:40Z

devito/passes/clusters/factorization.py


+    # Any factorization possible?
+    if len(inverse_mapper) == len(expr.args) or \
+       list(inverse_mapper) == [1]:


avoid list on (potentially large) set and len(inverse_mapper) == 1 && 1 in inverse_mapper

mloubout · 2024-08-21T12:31:55Z

devito/passes/clusters/factorization.py

    elif expr.is_Equality:
-        args, candidates = zip(*[_collect_nested(expr.lhs),
-                                 _collect_nested(expr.rhs)])
+        args, candidates = zip(*[_collect_nested(expr.lhs, strategy),


why do we need to run it on lhs?

this is legacy, I can probably drop it; will try

mloubout · 2024-08-21T12:32:40Z

devito/passes/clusters/utils.py

-           'in_critical_region']
-
-
-def makeit_ssa(exprs):


devito/symbolics/manipulation.py

devito/types/parallel.py

review-notebook-app · 2024-08-21T12:46:25Z

Check out this pull request on

See visual diffs & provide feedback on Jupyter Notebooks.

Powered by ReviewNB

georgebisbas · 2024-08-24T09:36:04Z

devito/core/gpu.py

@@ -196,14 +198,14 @@ def _specialize_clusters(cls, clusters, **kwargs):

        # Reduce flops


Flops minimization and Reduce flops as comments subsections looks duplicate....
I would keep CSE in the first subtitle, line 49

georgebisbas · 2024-08-24T09:45:31Z

examples/performance/01_gpu.ipynb

so, it is not applied here now

georgebisbas · 2024-08-24T09:46:41Z

tests/test_cse.py

+
+
+@pytest.mark.parametrize('exprs,expected,min_cost', [
+    # Simple cases


are all these just moved, or edited/enhanced as well?

very few modifications

georgebisbas · 2024-08-27T09:03:43Z

examples/performance/01_gpu.ipynb

@@ -307,8 +307,7 @@
      "    {\n",


so there is diff here, no temp?

Reply via ReviewNB

EdCaunt · 2024-09-02T14:22:27Z

devito/core/gpu.py

@@ -196,14 +198,14 @@ def _specialize_clusters(cls, clusters, **kwargs):

        # Reduce flops


EdCaunt · 2024-09-02T14:23:05Z

devito/core/operator.py

            raise InvalidOperator("Unsupported MPI mode `%s`" % oo['mpi'])

+        if oo['cse-algo'] not in ('basic', 'smartsort', 'advanced'):
+            raise InvalidArgument("Illegal `cse-algo` value")


This error message should say what the illegal value was, much like the one above

EdCaunt · 2024-09-02T14:24:57Z

devito/passes/clusters/cse.py

+    -----
+    We're not using SymPy's CSE for three reasons:
+
+        * It also captures array index access functions (e.g., i+1 in A[i+1]);


We will also probably want a unique kind of variable name to improve readability. Something like i0 rather than r0

EdCaunt · 2024-09-02T14:27:40Z

devito/passes/clusters/cse.py

+
+def _compact(exprs, exclude):
+    """
+    Drop useless temporaries:


Are there also edge cases where it creates expressions like r1 = r0? I'm sure we had some issue like this

One of the roles of _compact is catching them all !

FabioLuporini added the compiler label Aug 21, 2024

FabioLuporini requested review from EdCaunt, georgebisbas and mloubout August 21, 2024 10:04

EdCaunt reviewed Aug 21, 2024

View reviewed changes

mloubout reviewed Aug 21, 2024

View reviewed changes

FabioLuporini force-pushed the cse-tuplets branch 2 times, most recently from 8f15886 to 8171523 Compare August 21, 2024 14:10

FabioLuporini added 16 commits August 22, 2024 08:25

compiler: Fixup factorization

52b5191

tests: Move CSE tests into a separate module

7d14a06

compiler: Add skeleton of cse_tuplets

d9cb86f

compiler: Draft tuplets-based CSE algo

e720377

compiler: Enhance and fix tuplets-based CSE algo

fbbbc5b

compiler: Refine CSE and factorization algo selection

910129c

compiler: Relax detect_accesses

7232dfc

compiler: Drop useless makeit_ssa

60aeb31

compiler: Improve CSE's compact()

5df6a90

compiler: Fix CSE's smartsort determinism

eda1b70

api: Change nthreads default value

370c3a1

pep8 happiness

91f04ca

compiler: Fixup factorization setup

469aee2

examples: Update expected output

b7186eb

tests: Relax tolerance

ad5fd14

compiler: Polish factorizer

b1a10ee

FabioLuporini force-pushed the cse-tuplets branch from 0a3a196 to b1a10ee Compare August 22, 2024 08:25

georgebisbas reviewed Aug 24, 2024

View reviewed changes

mloubout approved these changes Aug 26, 2024

View reviewed changes

georgebisbas reviewed Aug 27, 2024

View reviewed changes

EdCaunt reviewed Sep 2, 2024

View reviewed changes

EdCaunt approved these changes Sep 2, 2024

View reviewed changes

FabioLuporini merged commit f012589 into master Sep 2, 2024

FabioLuporini deleted the cse-tuplets branch September 2, 2024 14:50


		mapper[Candidate(expr)].append(expr)

		for n in range(2, len(expr.args)):

		@@ -196,14 +198,14 @@ def _specialize_clusters(cls, clusters, **kwargs):

		# Reduce flops



		@pytest.mark.parametrize('exprs,expected,min_cost', [
		# Simple cases

compiler: Add various performance optimization variants #2446

compiler: Add various performance optimization variants #2446

Uh oh!

Conversation

FabioLuporini commented Aug 21, 2024

Uh oh!

codecov bot commented Aug 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mloubout left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

review-notebook-app bot commented Aug 21, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

georgebisbas Aug 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

codecov bot commented Aug 21, 2024 •

edited

Loading

georgebisbas Aug 27, 2024 •

edited

Loading