llvm / llvm/eudsl

[eudsl-llvmpy] Design: a canonical live-value registry (dedup + identity + invalidation) for Value/Instruction/BasicBlock wrappers

Open
#615 0 comments 0 reactions 0 assignees View on GitHub

Nobody has claimed this yet.

Dominant language
Python
Stars
79
Forks
14
Avg merge
11h 43m
Merged PRs (30d)
72

Description

## Context

eudsl-llvmpy currently binds `Value` and its subclasses (`Instruction`, `BasicBlock`, `Argument`, ...) as **thin, non-owning, un-deduped pointer views**: `EUDSL_CAST_CTOR` wraps a raw `llvm::Value*` with `rv_policy::reference` + `keep_alive<0,1>`, and there is no per-value registry. Only `Context` and `Module` have real lifetime tracking (what `assert_no_leaks()` counts). Everything below rides on `keep_alive` tying a value's lifetime to its parent.

This is a deliberate, cheap design that matches the "mirror the LLVM C++ API 1:1" goal. But it means the Python layer has no notion of *which* wrapper corresponds to a given C++ entity, so it cannot dedup wrappers, cannot offer stable identity, and cannot invalidate a wrapper when the underlying object goes away — except the one narrow case we've already patched.

This issue is a **design note to decide whether to build a canonical live-value registry**, analogous to MLIR's `PyOperation` + `liveOperations` system (`mlir/lib/Bindings/Python/IRCore.cpp`: `PyOperation::forOperation`, the `valid`/`checkValid()`/`setInvalid()` flag, and transitive invalidation on erase).

## What we already fixed (for reference)

`Instruction.erase_from_parent` / `BasicBlock.erase_from_parent` now poison the *called* wrapper via `nb::inst_set_state(self, ready=false, destruct=false)` after freeing the C++ object, so a use-after-erase on that handle raises `TypeError` ("attempted to access an uninitialized instance") instead of dereferencing freed memory (PR #608). This handles the **common footgun** (erase then touch the same handle).

## Gaps that remain (what a registry would close)

| Gap | Severity | Needs a registry? |
|---|---|---|
| erase-then-touch the **same** handle | common | already fixed (poison) |
| erase-then-touch an **aliased** handle (a second wrapper for the same value, e.g. via `maybe_downcast` re-wrap) | rare | yes |
| stable identity (`inst is inst` across two lookups) | quality-of-life | yes |
| transitive invalidation (erase a `BasicBlock` -> wrappers for its instructions dangle) | real but narrow | yes |

Only the bottom three need the machinery, and all three are lower-frequency than the one already handled.

## What MLIR does (the model to consider)

- **Canonical dedup:** one `PyOperation` per C++ operation, via a `liveOperations` map keyed by raw pointer (`PyOperation::forOperation`). Guarantees identity and gives a single place to invalidate that reaches every alias.
- **Validity flag:** `PyOperation::valid` + `checkValid()` throws on any access after invalidation; `erase()` does `checkValid(); setInvalid(); mlirOperationDestroy(...)`.
- **Transitive invalidation:** erasing/attaching invalidates dependent wrappers (subtree, symbol-table erase, etc.).

MLIR pays for this with a map lookup+insert on **every** wrapper creation and per-wrapper context-ref + validity + attachment state.

## Options

1. **Do nothing more.** Keep the documented caveat: poison covers the common case; aliasing/transitive are rare and documented. Lowest cost; matches the thin-mirror design.
2. **Add a scoped-down `liveValues` registry** keyed by `llvm::Value*`, giving dedup + `is` identity + a single poison point on erase (and a hook for transitive invalidation). This is the "do it properly" option.
3. **Full MLIR-style system** with attachment tracking and transitive subtree invalidation. Largest change.

## Cost / risks of a registry (options 2-3)

- LLVM churns `Instruction`/`BasicBlock` objects far more than MLIR churns ops, so a registry keyed on every value adds lookup/insert overhead on a hot path.
- **Thread-safety:** an unsynchronized registry on the hot path re-raises exactly the free-threaded data-race concern tracked in #614 (the `casterMap()` + Python-pass registries). If we build a live-value registry it should be designed **together with** the #614 synchronization work — same shape of problem, same solution surface.
- Correctness edge cases remain regardless: LLVM RAUW, `moveBefore`, `splitBasicBlock` all mutate the graph under existing wrappers.

## Recommendation (for discussion)

Lean **option 1 now**; pursue **option 2 only if real usage surfaces identity/aliasing/transitive bugs**. If we do option 2, co-design it with #614 so the registry is synchronized from the start.

## References
- `projects/eudsl-llvmpy/src/IR/Common.h` — `EUDSL_CAST_CTOR` (thin pointer-view wrappers)
- `projects/eudsl-llvmpy/src/IR/Casters.cpp` — `casterMap()` / `maybe_downcast` (the only re-wrap path, a source of aliases)
- `projects/eudsl-llvmpy/src/IR/Values.cpp` — `erase_from_parent` poisoning (PR #608)
- MLIR: `mlir/lib/Bindings/Python/IRCore.cpp` — `PyOperation::forOperation`, `checkValid`/`setInvalid`, `liveOperations`
- Related: #614 (free-threaded data races in unsynchronized static registries) — cross-linked; a live-value registry would share its synchronization design.

Contributor guide

No contributing guide indexed for this repository

First steps

  1. Read the whole issue, then the project's contributing guide.
  2. Comment on the issue to say you are picking it up — it saves two people doing the same work.
  3. Fork the repository and make your change on a branch.
  4. Open a pull request that references the issue number.

Research direction

Start with projects/eudsl-llvmpy/src/IR/Common.h, Casters.cpp, and Values.cpp, then compare the referenced MLIR IRCore.cpp design and related issue #614. Evaluate the three registry options against aliasing, invalidation, performance, and synchronization concerns. Done means a documented decision with agreed scope and follow-up criteria, not an implementation patch.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp, python
Domain
compilers, devtools
Issue type
Feature
Difficulty
5/5
Estimated time
Over a week
Activity status
Active
Clarity
Needs clarification
Newbie friendliness
30/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.