Skip to content

Order free indices by creation, not by memory address - #291

Merged
pbrubeck merged 1 commit into
mainfrom
pbrubeck/gem-deterministic-index-order
Sep 7, 2026
Merged

pbrubeck merged 1 commit into
mainfrom
pbrubeck/gem-deterministic-index-order

Conversation

@pbrubeck

@pbrubeck pbrubeck commented Sep 6, 2026

Copy link
Copy Markdown

gem.unique sorted free indices with key=id, and Index.__lt__ compared
addresses. Every GEM node routes its free_indices through unique
(NodeMeta.__call__), so the index order of a whole expression tree followed
the addresses CPython happened to hand out. Two processes compiling the same
form could lay the same loop nest out differently and emit different code.

Under MPI that difference deadlocks Firedrake. The generated code feeds
LocalKernel._immutable_cache_key, which feeds GlobalKernel.cache_key, so
ranks that laid a nest out differently disagree about the key for PyOP2's
in-memory compile_global_kernel cache. PyOP2 reconciles a hit/miss
disagreement for disk caches but assumes in-memory caches always agree, so the
ranks that miss descend alone into the disk cache's collective broadcast and
wait there for the ranks that hit and moved on. A 32-rank Firedrake multigrid
run on a cold cache hung there; with this branch it completes.

There are two sites, not one. unique is the obvious one. The other is
replace_indices_indexed in gem/optimise.py, which sorts substitution pairs
keyed on an Index and so ordered them by address through Index.__lt__.

Index.count is assigned in creation order and is unique within a process, so
ordering on it gives every process the same order.

What is still not deterministic

This makes the ordering of indices reproducible across processes. It does not
make GEM's hashing reproducible, and that distinction is worth stating plainly
because the two are easy to conflate.

Index.__hash__ is still inherited from object, so it derives from the
address. Any set or dict of indices that is iterated without sorting therefore
still iterates in a process-dependent order. The same is true more widely:
VariableIndex.__hash__ is hash((type(self), self.expression)) and every
expression node hashes through Node.get_hash, and hash(type) is itself
identity-based.

Keying Index.__hash__ on count was tried and deliberately left out, for two
reasons. It breaks test_macro_multigrid_biharmonic[HCT] and [HCT-red] in
Firedrake, which start failing an internal TSFC assertion on leftover free
indices. And it would not buy determinism in general anyway: hash of a str
is randomised per process, so neither hash((type(self), count)) nor
hash((type(self).__name__, count)) is stable across processes, and only a
bare hash(count) is. UFL has the same hole — Index.__hash__ there is
hash(("Index", self._count)), which differs between processes for the same
count — though UFL is immune to the bug fixed here, because its equality and
ordering are count-based rather than address-based.

So this branch closes the ordering hole that was causing a hang, and leaves the
hashing question open.

Testing

test/gem/test_index.py is new; both tests fail on main and pass here.
make srclint is clean, and the FIAT, FInAT and GEM suites pass.

On the Firedrake side, tests/firedrake/{tsfc,multigrid,macro} was run paired
against main's GEM — both variants warmed, separate caches, failures compared
by name — and the failure sets are identical, so nothing here regresses.

Assisted by Claude Code (Claude Opus 5).

@pbrubeck
pbrubeck force-pushed the pbrubeck/gem-deterministic-index-order branch 2 times, most recently from 2e84a89 to 6644978 Compare September 6, 2026 16:34
gem.unique sorted free indices with key=id, and Index.__lt__ compared
addresses. Every GEM node routes its free_indices through unique, so the
index order of a whole expression tree followed the addresses CPython
happened to hand out, and replace_indices_indexed sorts substitution pairs
on the same comparison. Two processes compiling the same form could lay the
same loop nest out differently and emit different code.

Under MPI that difference deadlocks Firedrake. The generated code feeds
LocalKernel._immutable_cache_key, which feeds GlobalKernel.cache_key, so
ranks that laid a nest out differently disagree about the key for PyOP2's
in-memory compile_global_kernel cache. PyOP2 reconciles a hit/miss
disagreement for disk caches but assumes in-memory caches always agree, so
the ranks that miss descend alone into the disk cache's collective
broadcast and wait there for the ranks that hit and moved on.

Index.count is assigned in creation order and is unique within a process,
so ordering on it gives every process the same order.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@pbrubeck
pbrubeck force-pushed the pbrubeck/gem-deterministic-index-order branch from 6644978 to 30f1eac Compare September 6, 2026 16:40
@pbrubeck
pbrubeck requested a review from connorjward September 7, 2026 07:33

@connorjward connorjward left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

@JHopeCollins this may be the cause of that code generation issue you had with asQ all that time ago.

Comment thread gem/gem.py
def __lt__(self, other):
# Allow sorting of free indices in Python 3
return id(self) < id(other)
return self.count < other.count

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

OK this is completely mental

@pbrubeck
pbrubeck merged commit d0f9589 into main Sep 7, 2026
9 checks passed
@pbrubeck
pbrubeck deleted the pbrubeck/gem-deterministic-index-order branch September 7, 2026 08:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants