[eudsl-llvmpy] Design: a canonical live-value registry (dedup + identity + invalidation) for Value/Instruction/BasicBlock wrappers
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
- Read the whole issue, then the project's contributing guide.
- Comment on the issue to say you are picking it up — it saves two people doing the same work.
- Fork the repository and make your change on a branch.
- 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