Skip to content

[MLIR][Python] make Sliceable inherit from Sequence #170551

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
makslevental merged 1 commit into from
Dec 5, 2025
+15 −1
Merged

[MLIR][Python] make Sliceable inherit from Sequence #170551

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion mlir/lib/Bindings/Python/NanobindUtils.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
#include "llvm/Support/raw_ostream.h"

#include <string>
#include <typeinfo>
#include <variant>

template <>
Expand Down Expand Up @@ -344,7 +345,16 @@ class Sliceable {

/// Binds the indexing and length methods in the Python class.
static void bind(nanobind::module_ &m) {
auto clazz = nanobind::class_<Derived>(m, Derived::pyClassName)
const std::type_info &elemTy = typeid(ElementTy);
PyObject *elemTyInfo = nanobind::detail::nb_type_lookup(&elemTy);
assert(elemTyInfo &&
"expected nb_type_lookup to succeed for Sliceable elemTy");
Comment on lines +351 to +352
Copy link
Contributor Author

@makslevental makslevental Dec 4, 2025

Choose a reason for hiding this comment

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

There's no way for this to fail - the ElementTy needs to be registered before Sliceable[ElementTy] can be registered...

nanobind::handle elemTyName = nanobind::detail::nb_type_name(elemTyInfo);
std::string sig = std::string("class ") + Derived::pyClassName +
"(collections.abc.Sequence[" +
nanobind::cast<std::string>(elemTyName) + "])";
Copy link
Member

@ftynse ftynse Dec 5, 2025

Choose a reason for hiding this comment

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

Don't we have ElementTy::pyClassName similarly to Derived::pyClassName? Maybe not everywhere, but something to consider. This is probably the first time I see typeid being used in non-toy code so I don't know how well it works across platforms.

Copy link
Contributor Author

@makslevental makslevental Dec 5, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Don't we have ElementTy::pyClassName similarly to Derived::pyClassName?

No:

llvm-project/mlir/lib/Bindings/Python/IRModule.h

Line 759 in 4ada42e

class PyRegion {

This is probably the first time I see typeid being used in non-toy code so I don't know how well it works across platforms.

You realize that all of nanobind/pybind hinges on typeid right?

https://github.com/search?q=repo%3Awjakob%2Fnanobind%20typeid&type=code

Specifically the core impl (the cpp to python mapping) uses std::type_info

https://github.com/wjakob/nanobind/blob/892dd8173dd6fad4ec3a50eaa51863ae45c3205d/src/nb_internals.h#L381

https://github.com/wjakob/nanobind/blob/892dd8173dd6fad4ec3a50eaa51863ae45c3205d/include/nanobind/nb_class.h#L564

Ie that's why RTTI is on for the bindings. So we can be sure it works in all the places the bindings work 🙂.

auto clazz = nanobind::class_<Derived>(m, Derived::pyClassName,
nanobind::sig(sig.c_str()))
.def("__add__", &Sliceable::dunderAdd);
Derived::bindDerived(clazz);

Expand Down
4 changes: 4 additions & 0 deletions mlir/test/python/ir/operation.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,10 @@ def testTraverseOpRegionBlockIterators():
)
op = module.operation
assert op.context is ctx
# Note, __nb_signature__ stores the fully-qualified signature - the actual type stub emitted is
# class RegionSequence(Sequence[Region])
# CHECK: class RegionSequence(collections.abc.Sequence[mlir._mlir_libs._mlir.ir.Region])
print(RegionSequence.__nb_signature__)
Copy link
Contributor

@ingomueller-net ingomueller-net Dec 5, 2025

Choose a reason for hiding this comment

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

Oh, that's a nice way to test this!

# Get the block using iterators off of the named collections.
regions = list(op.regions[:])
blocks = list(regions[0].blocks)
Expand Down
Loading