compiler: Address various compiler hotspots with operators containing large expression counts #2624

EdCaunt · 2025-06-06T15:54:12Z

Now ready for reviews.

codecov · 2025-06-06T16:12:03Z

Codecov Report

Attention: Patch coverage is 77.77778% with 6 lines in your changes missing coverage. Please review.

Project coverage is 92.00%. Comparing base (b3408d8) to head (613f1fe).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
devito/tools/utils.py	75.00%	3 Missing and 1 partial ⚠️
devito/ir/support/basic.py	0.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2624      +/-   ##
==========================================
- Coverage   92.01%   92.00%   -0.01%     
==========================================
  Files         245      245              
  Lines       48710    48727      +17     
  Branches     4292     4294       +2     
==========================================
+ Hits        44819    44830      +11     
- Misses       3205     3209       +4     
- Partials      686      688       +2

Flag	Coverage Δ
pytest-gpu-aomp-amdgpuX	`72.59% <77.77%> (-0.01%)`	⬇️
pytest-gpu-nvc-nvidiaX	`73.66% <77.77%> (-0.01%)`	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

JDBetteridge · 2025-06-11T09:36:32Z

devito/ir/support/basic.py

        return self.timestamp < other.timestamp

+    # Note: memoization yields mild compiler speedup
+    @memoized_meth


Where did this bad boy come from, and can we not use the built in @cached/@lru_cache? I believe these are thread safe

Looks like memoized_meth caches on the instance, whereas @cache I believe would have some unexpected behavior for instance methods (possibly would store an extra reference?)

Yes, iirc you need to be careful using @cache and @lru_cache on methods, as you can leak memory if you just use them as-is

Okay, I will update this comment to say that this cache is not thread safe

memoization yields mild compiler speedup.

how mild? if it's just 1~2% we may as well avoid it

Honestly probably something around that? It was just enough to be consistently visible above the noise across multiple runs

JDBetteridge · 2025-06-11T09:39:20Z

devito/symbolics/queries.py

+        return expr.is_CTemp
+    except AttributeError:
+        return False
+


Does this really need to be a stand alone function?

getattr(expr, "is_CTemp", False)

Keeps consistency with the other search methods which improves readability, so I would say yes

Could have just done isinstance like for ComponentAccess rather than the is_CTemp that's used only here since it's not "standard" sympy is_xxx

JDBetteridge · 2025-06-11T09:40:24Z

devito/symbolics/search.py


+def retrieve_ctemps(exprs, mode='all'):
+    """Shorthand to retrieve the CTemps in `exprs`"""
+    return search(exprs, q_ctemp, mode, 'dfs')


If this is the only place the function is used, it can be a lambda

It can, but imo it's better to keep consistency with the other search shortcuts

I agree about consistency.

However, since ctemps are a CSE-local type, I'd move it inside cse.py

mloubout

Minor comments, thanks for digging.

mloubout · 2025-06-11T12:40:23Z

devito/ir/support/basic.py

+                # To avoid evaluating expensive symbolic Lt or Gt operations,
+                # we pre-empt such operations by checking if the values to be compared
+                # to are symbolic, and skip this case if not.
+                if not any(isinstance(i, sympy.core.Basic)


just sympy.Basic

mloubout · 2025-06-11T12:42:52Z

devito/passes/clusters/cse.py

-    mapper.update({e.lhs: e.rhs for e in candidates
-                   if sum([i.rhs.count(e.lhs) for i in exprs]) == 1})
+    # Find all the CTemps in expression right-hand-sides without removing duplicates
+    ctemps = retrieve_ctemps([e.rhs for e in exprs])


(e.rhs for e in expr) don't need a list here just generator

mloubout · 2025-06-11T12:43:09Z

devito/passes/clusters/cse.py

-                   if sum([i.rhs.count(e.lhs) for i in exprs]) == 1})
+    # Find all the CTemps in expression right-hand-sides without removing duplicates
+    ctemps = retrieve_ctemps([e.rhs for e in exprs])
+    ctemp_count = Counter(ctemps)


move in if ctemps

mloubout · 2025-06-11T12:44:23Z

devito/symbolics/queries.py

+        return expr.is_CTemp
+    except AttributeError:
+        return False
+


Could have just done isinstance like for ComponentAccess rather than the is_CTemp that's used only here since it's not "standard" sympy is_xxx

FabioLuporini · 2025-06-11T13:07:25Z

devito/passes/clusters/misc.py

                # (intuitively, "the loop nests are to be kept separated")
                # * All ClusterGroups between `cg0` and `cg1` must precede `cg1`
                # * All ClusterGroups after `cg1` cannot precede `cg1`
+


why a blank line?

FabioLuporini · 2025-06-11T14:54:05Z

devito/ir/support/basic.py

+                # To avoid evaluating expensive symbolic Lt or Gt operations,
+                # we pre-empt such operations by checking if the values to be compared
+                # to are symbolic, and skip this case if not.
+                if not any(isinstance(i, sympy.Basic)


do you mean if all(is_integer(...)) ?

Are the two statements equivalent here?

Certainly the current implementation is more congruent with the comment and my original intention

FabioLuporini · 2025-06-11T14:54:30Z

devito/ir/support/basic.py

+                           for i in (sit.symbolic_min, sit.symbolic_max)):
+
+                    for v in (self[n], other[n]):
+                        try:


I don't think you need the try-except anymore now that you have the if on the outside?

Symbolic inequalities cannot always be converted to bool, so the try-except is still needed since v can still be symbolic I think

FabioLuporini · 2025-06-12T07:42:23Z

devito/ir/support/basic.py

-                            return Vector(S.ImaginaryUnit)
-                    except TypeError:
-                        pass
+                    # Note: To avoid evaluating expensive symbolic Lt or Gt operations,


this comment is now obsolete and should be dropped

smart_lt's docstring should already contain all this info, no?

FabioLuporini

GTG

…re try-except

EdCaunt requested a review from FabioLuporini June 6, 2025 15:54

EdCaunt self-assigned this Jun 6, 2025

EdCaunt added WIP Still work in progress compiler labels Jun 6, 2025

EdCaunt force-pushed the compiler-speed branch from feb82b7 to b2c15c3 Compare June 9, 2025 12:59

EdCaunt requested review from georgebisbas and mloubout June 11, 2025 08:14

EdCaunt force-pushed the compiler-speed branch 2 times, most recently from 9b01400 to 8377718 Compare June 11, 2025 08:49

EdCaunt changed the title ~~compiler: Address various compiler hotspots with operators containing large expression counts [WIP]~~ compiler: Address various compiler hotspots with operators containing large expression counts Jun 11, 2025

JDBetteridge requested changes Jun 11, 2025

View reviewed changes

mloubout reviewed Jun 11, 2025

View reviewed changes

FabioLuporini reviewed Jun 11, 2025

View reviewed changes

EdCaunt removed the WIP Still work in progress label Jun 11, 2025

EdCaunt force-pushed the compiler-speed branch from 3048ed2 to 9263938 Compare June 11, 2025 13:23

EdCaunt requested review from FabioLuporini, JDBetteridge, enwask and mloubout June 11, 2025 13:53

JDBetteridge approved these changes Jun 11, 2025

View reviewed changes

FabioLuporini reviewed Jun 11, 2025

View reviewed changes

EdCaunt force-pushed the compiler-speed branch from 0e1083b to 7c1b546 Compare June 11, 2025 17:13

FabioLuporini reviewed Jun 12, 2025

View reviewed changes

EdCaunt requested a review from FabioLuporini June 13, 2025 09:16

FabioLuporini approved these changes Jun 13, 2025

View reviewed changes

mloubout approved these changes Jun 13, 2025

View reviewed changes

EdCaunt added 2 commits June 13, 2025 12:59

compiler: Pull some aspects of DAG construction out and cache

d36342b

compiler: Tweaks to CTemp purging in CSE

90ba30e

EdCaunt added 12 commits June 13, 2025 12:59

compiler: Tidy up new searches and functionality tweaks

d165087

compiler: Use counter

1c96965

compiler: Revert incorrect modifications to DAG construction

2015111

compiler: Further misc improvements (minor)

ca79bee

compiler: Add a check to pre-empt expensive symbolic comparisons befo…

ad69ef7

…re try-except

misc: Refactoring and misc code style improvments

c725b2f

misc: Use less esoteric comment phrasing

89cde25

misc: Add comparisons that give up upon encountering symbolic arguments

3d6eff0

misc: Make avoid_symbolic decorator more generic

8022ca3

misc: Tweak avoid_symbolic decorator

d5e39cb

misc: Update comment

373990a

misc: Refactor and tidy decorator

613f1fe

mloubout force-pushed the compiler-speed branch from 97765a3 to 613f1fe Compare June 13, 2025 17:03

mloubout merged commit ed3585a into main Jun 13, 2025
35 checks passed

mloubout deleted the compiler-speed branch June 13, 2025 19:32

compiler: Address various compiler hotspots with operators containing large expression counts #2624

compiler: Address various compiler hotspots with operators containing large expression counts #2624

Uh oh!

Conversation

EdCaunt commented Jun 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Jun 6, 2025 • 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!

enwask Jun 11, 2025 • 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!

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!

mloubout left a comment

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!

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!

EdCaunt Jun 11, 2025 • 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!

FabioLuporini left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

EdCaunt commented Jun 6, 2025 •

edited

Loading

codecov bot commented Jun 6, 2025 •

edited

Loading

enwask Jun 11, 2025 •

edited

Loading

EdCaunt Jun 11, 2025 •

edited

Loading