Skip to content

compiler: Preserve compilation invariants under symbol renaming - #3084

Open
FabioLuporini wants to merge 14 commits into
mainfrom
kernel-local-names
Open

FabioLuporini wants to merge 14 commits into
mainfrom
kernel-local-names

Conversation

@FabioLuporini

Copy link
Copy Markdown
Contributor

Generated temporary names can affect CSE scheduling, while symbolic substitutions can reorder lowered arithmetic or leave stale references in auxiliary metadata. Application identifiers such as r0 can also collide with compiler-generated temporaries.

This PR addresses these issues and provides a final IET customization point for backend naming passes:

  • Schedule ready CSE temporaries in expression order, independently of their names.
  • Preserve lowered Add/Mul operand order and grouping during substitution, retaining numeric folding and handling nested negations correctly.
  • Substitute dereference offsets and flattened-array stride metadata while preserving scoped C member names.
  • Distinguish flattened-array macros by their stride bindings.
  • Reserve application and generated identifiers through SymbolRegistry.reserve.
  • Abstract equal symbol instances once, preserving equivalent helper reuse.
  • Add _finalize_iet after dtype lowering and symbol minimization, before macro extraction and signature finalization.

@codecov

codecov Bot commented Oct 22, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 53.45912% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 53.04%. Comparing base (6d2ad82) to head (0325dd2).

Files with missing lines Patch % Lines
tests/test_symbolics.py 24.39% 31 Missing ⚠️
tests/test_iet.py 18.75% 13 Missing ⚠️
tests/test_cse.py 30.76% 9 Missing ⚠️
tests/test_operator.py 20.00% 8 Missing ⚠️
devito/ir/support/symregistry.py 71.42% 2 Missing and 2 partials ⚠️
devito/passes/clusters/cse.py 0.00% 3 Missing ⚠️
devito/symbolics/manipulation.py 89.28% 2 Missing and 1 partial ⚠️
tests/test_dimension.py 0.00% 2 Missing ⚠️
tests/test_pickle.py 66.66% 1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6d2ad82) and HEAD (0325dd2). Click for more details.

HEAD has 11 uploads less than BASE
Flag BASE (6d2ad82) HEAD (0325dd2)
18 10
pytest-gpu-gcc- 6 4
pytest-gpu-icx- 3 2
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #3084       +/-   ##
===========================================
- Coverage   84.28%   53.04%   -31.25%     
===========================================
  Files         258      258               
  Lines       57469    57601      +132     
  Branches     4914     4918        +4     
===========================================
- Hits        48439    30553    -17886     
- Misses       8206    26019    +17813     
- Partials      824     1029      +205     
Flag Coverage Δ
pytest-gpu-aomp-amdgpuX 68.49% <76.19%> (-0.01%) ⬇️
pytest-gpu-gcc- 43.21% <45.28%> (-35.72%) ⬇️
pytest-gpu-icx- 43.06% <45.28%> (-35.79%) ⬇️
pytest-gpu-nvc-nvidiaX 69.22% <76.19%> (+0.02%) ⬆️

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

☔ View full report in Codecov by Harness.
📢 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.

@mloubout mloubout left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks sane

@review-notebook-app

Copy link
Copy Markdown

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

Comment thread devito/ir/support/symregistry.py Outdated
self.reserved.update(i.name for e in exprs for i in e.free_symbols)

# Before indexification, a Function's free symbols omit its own name
self.reserved.update(i.name for i in retrieve_functions(exprs))

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mode='unique'

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK

Comment thread devito/passes/clusters/cse.py
Comment thread devito/symbolics/manipulation.py Outdated
Comment thread tests/test_operator.py Outdated
Retrieve Functions uniquely when reserving names, share the default
uxreplace traversal through an explicit helper, and remove the unneeded
Operator name from the input-name collision test.

Keep the CSE position map to avoid repeated linear scans of exprs. In a
comparison of the actual topological sort with 128 independent CTemps,
including map construction, cached lookup took 39 ms versus 1255 ms for
list.index. Add a comment explaining the cached lookup.

Validation: focused C/OpenMP regressions (64 passed, 1 xfailed) and
C++/OpenMP regressions (62 passed, 1 xfailed). The full CPU audit found
separate numerical failures already present before the rebase.
Comment thread devito/ir/cgen/printer.py

def __init__(self, settings=None):
super().__init__(settings)
self._order = self.order

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded

Comment thread devito/ir/cgen/printer.py

def __init__(self, settings=None):
super().__init__(settings)
self._order = self.order

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seconded



@_uxreplace_dispatch.register(dict)
@_uxreplace_dispatch.register(frozendict)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dispatcher currently turns a frozendict into a dict. You need to add a cast at the end to make sure it's the correct type.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixing

Comment on lines +220 to +221
@_uxreplace_handle.register(UnevalAdd)
@_uxreplace_handle.register(UnevalMul)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not register UnevalMixin instead of having multiple registrations here?

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants