[llvm] [mlir] [Python] Develop python bindings for Presburger library (PR #113233)

Maksim Levental via llvm-commits llvm-commits at lists.llvm.org
Thu Nov 14 11:35:45 PST 2024


makslevental wrote:

> I don't see how the tableau object could be eliminated if we want to keep the `relation.equalities[...]` API

Yes I'm proposing that the API be eliminated/hoisted. 

Note, I'm not proposing it be eliminated on principled grounds, like that there is no `PresburgerTableau` in the `affine` namespace and thus this would not be a binding but an additional feature (something that, prior, others have [blocked](https://github.com/llvm/llvm-project/pull/79449#issuecomment-1916347887)). 

I'm proposing it be hoisted into `PyPresburgerIntegerRelation` for the sake of simplicity and to moot the concern of ownership/lifetime, which is a much stronger concern here (in these bindings) where we've had and continue to have many issues around lifetime of objects passed from C++ to Python ([1](https://github.com/llvm/llvm-project/pull/93339), [2](https://github.com/llvm/llvm-project/issues/69730), [3](https://github.com/llvm/llvm-project/issues/92344), last one being an issue I believe you raised...).

> I think the current implementation is correct in terms of ensuring that PyPresburgerRelation should not be freed by Python while any connected PyPresburgerTableau are live.

In fact the "correct" thing to do, if we decide the API is indeed worth the cognitive and maintenance cost, is to use [`return_value_policy::reference_internal`](https://pybind11.readthedocs.io/en/stable/advanced/functions.html#return-value-policies):

> Indicates that the lifetime of the return value is tied to the lifetime of a parent object, namely the implicit this, or self argument of the called method or property. Internally, this policy works just like `return_value_policy::reference` but additionally applies a `keep_alive<0, 1>` call policy (described in the next section) that prevents the parent object from being garbage collected as long as the return value is referenced by Python. This is the default policy for property getters created via `def_property`, `def_readwrite`, etc.

Or (as that doc alludes) to make this a `def_property` instead of `def` on `PyPresburgerIntegerRelation`.

https://github.com/llvm/llvm-project/pull/113233


More information about the llvm-commits mailing list