Skip to content

[mlir python] Port in-tree dialects to nanobind. #119924

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.

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
merged 2 commits into from
Dec 21, 2024
Merged

Conversation

hawkinsp
Copy link
Contributor

@hawkinsp hawkinsp commented Dec 13, 2024

This is a companion to #118583, although it can be landed independently because since #117922 dialects do not have to use the same Python binding framework as the Python core code.

This PR ports all of the in-tree dialect and pass extensions to nanobind, with the exception of those that remain for testing pybind11 support. It would make sense to merge this PR after merging #118583, if we have agreed that we are migrating the core to nanobind.

This PR also:

  • removes CollectDiagnosticsToStringScope from NanobindAdaptors.h. This was overlooked in a previous PR and it is duplicated in Diagnostics.h.

@llvmbot llvmbot added mlir:python MLIR Python bindings mlir bazel "Peripheral" support tier build system: utils/bazel labels Dec 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 13, 2024

@llvm/pr-subscribers-mlir

Author: Peter Hawkins (hawkinsp)

Changes

This is a companion to #118583, although it can be landed independently because since #117922 dialects do not have to use the same Python binding framework as the Python core code.

This PR ports all of the in-tree dialect and pass extensions to nanobind, with the exception of those that remain for testing pybind11 support. It would make sense to merge this PR after merging #118583, if we have agreed that we are migrating the core to nanobind.

This PR also:

  • removes CollectDiagnosticsToStringScope from NanobindAdaptors.h. This was overlooked in a previous PR and it is duplicated in Diagnostics.h.

Patch is 63.07 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/119924.diff

19 Files Affected:

  • (modified) mlir/include/mlir/Bindings/Python/NanobindAdaptors.h (-34)
  • (modified) mlir/lib/Bindings/Python/AsyncPasses.cpp (+2-3)
  • (modified) mlir/lib/Bindings/Python/DialectGPU.cpp (+24-22)
  • (modified) mlir/lib/Bindings/Python/DialectLLVM.cpp (+33-25)
  • (modified) mlir/lib/Bindings/Python/DialectLinalg.cpp (+7-5)
  • (modified) mlir/lib/Bindings/Python/DialectNVGPU.cpp (+11-10)
  • (modified) mlir/lib/Bindings/Python/DialectPDL.cpp (+21-23)
  • (modified) mlir/lib/Bindings/Python/DialectQuant.cpp (+37-38)
  • (modified) mlir/lib/Bindings/Python/DialectSparseTensor.cpp (+21-21)
  • (modified) mlir/lib/Bindings/Python/DialectTransform.cpp (+24-25)
  • (modified) mlir/lib/Bindings/Python/ExecutionEngineModule.cpp (+46-42)
  • (modified) mlir/lib/Bindings/Python/GPUPasses.cpp (+2-3)
  • (modified) mlir/lib/Bindings/Python/LinalgPasses.cpp (+2-2)
  • (modified) mlir/lib/Bindings/Python/RegisterEverything.cpp (+3-2)
  • (modified) mlir/lib/Bindings/Python/SparseTensorPasses.cpp (+2-2)
  • (modified) mlir/lib/Bindings/Python/TransformInterpreter.cpp (+22-21)
  • (modified) mlir/python/CMakeLists.txt (+15)
  • (modified) mlir/test/python/dialects/sparse_tensor/dialect.py (+1-1)
  • (modified) utils/bazel/llvm-project-overlay/mlir/BUILD.bazel (+12-11)
diff --git a/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h b/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
index 5e01cebcb09c91..71b12b4dfc637f 100644
--- a/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
+++ b/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
@@ -631,40 +631,6 @@ class mlir_value_subclass : public pure_subclass {
 
 } // namespace nanobind_adaptors
 
-/// RAII scope intercepting all diagnostics into a string. The message must be
-/// checked before this goes out of scope.
-class CollectDiagnosticsToStringScope {
-public:
-  explicit CollectDiagnosticsToStringScope(MlirContext ctx) : context(ctx) {
-    handlerID = mlirContextAttachDiagnosticHandler(ctx, &handler, &errorMessage,
-                                                   /*deleteUserData=*/nullptr);
-  }
-  ~CollectDiagnosticsToStringScope() {
-    assert(errorMessage.empty() && "unchecked error message");
-    mlirContextDetachDiagnosticHandler(context, handlerID);
-  }
-
-  [[nodiscard]] std::string takeMessage() { return std::move(errorMessage); }
-
-private:
-  static MlirLogicalResult handler(MlirDiagnostic diag, void *data) {
-    auto printer = +[](MlirStringRef message, void *data) {
-      *static_cast<std::string *>(data) +=
-          llvm::StringRef(message.data, message.length);
-    };
-    MlirLocation loc = mlirDiagnosticGetLocation(diag);
-    *static_cast<std::string *>(data) += "at ";
-    mlirLocationPrint(loc, printer, data);
-    *static_cast<std::string *>(data) += ": ";
-    mlirDiagnosticPrint(diag, printer, data);
-    return mlirLogicalResultSuccess();
-  }
-
-  MlirContext context;
-  MlirDiagnosticHandlerID handlerID;
-  std::string errorMessage = "";
-};
-
 } // namespace python
 } // namespace mlir
 
diff --git a/mlir/lib/Bindings/Python/AsyncPasses.cpp b/mlir/lib/Bindings/Python/AsyncPasses.cpp
index b611a758dbbb37..540fbf5bbe7290 100644
--- a/mlir/lib/Bindings/Python/AsyncPasses.cpp
+++ b/mlir/lib/Bindings/Python/AsyncPasses.cpp
@@ -8,14 +8,13 @@
 
 #include "mlir-c/Dialect/Async.h"
 
-#include <pybind11/detail/common.h>
-#include <pybind11/pybind11.h>
+#include <nanobind/nanobind.h>
 
 // -----------------------------------------------------------------------------
 // Module initialization.
 // -----------------------------------------------------------------------------
 
-PYBIND11_MODULE(_mlirAsyncPasses, m) {
+NB_MODULE(_mlirAsyncPasses, m) {
   m.doc() = "MLIR Async Dialect Passes";
 
   // Register all Async passes on load.
diff --git a/mlir/lib/Bindings/Python/DialectGPU.cpp b/mlir/lib/Bindings/Python/DialectGPU.cpp
index 560a54bcd15919..f0154f1b110bdc 100644
--- a/mlir/lib/Bindings/Python/DialectGPU.cpp
+++ b/mlir/lib/Bindings/Python/DialectGPU.cpp
@@ -9,21 +9,23 @@
 #include "mlir-c/Dialect/GPU.h"
 #include "mlir-c/IR.h"
 #include "mlir-c/Support.h"
-#include "mlir/Bindings/Python/PybindAdaptors.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
-#include <pybind11/detail/common.h>
-#include <pybind11/pybind11.h>
+#include <nanobind/nanobind.h>
+#include <nanobind/stl/optional.h>
+
+namespace nb = nanobind;
+using namespace nanobind::literals;
 
-namespace py = pybind11;
 using namespace mlir;
 using namespace mlir::python;
-using namespace mlir::python::adaptors;
+using namespace mlir::python::nanobind_adaptors;
 
 // -----------------------------------------------------------------------------
 // Module initialization.
 // -----------------------------------------------------------------------------
 
-PYBIND11_MODULE(_mlirDialectsGPU, m) {
+NB_MODULE(_mlirDialectsGPU, m) {
   m.doc() = "MLIR GPU Dialect";
   //===-------------------------------------------------------------------===//
   // AsyncTokenType
@@ -34,11 +36,11 @@ PYBIND11_MODULE(_mlirDialectsGPU, m) {
 
   mlirGPUAsyncTokenType.def_classmethod(
       "get",
-      [](py::object cls, MlirContext ctx) {
+      [](nb::object cls, MlirContext ctx) {
         return cls(mlirGPUAsyncTokenTypeGet(ctx));
       },
-      "Gets an instance of AsyncTokenType in the same context", py::arg("cls"),
-      py::arg("ctx") = py::none());
+      "Gets an instance of AsyncTokenType in the same context", nb::arg("cls"),
+      nb::arg("ctx").none() = nb::none());
 
   //===-------------------------------------------------------------------===//
   // ObjectAttr
@@ -47,12 +49,12 @@ PYBIND11_MODULE(_mlirDialectsGPU, m) {
   mlir_attribute_subclass(m, "ObjectAttr", mlirAttributeIsAGPUObjectAttr)
       .def_classmethod(
           "get",
-          [](py::object cls, MlirAttribute target, uint32_t format,
-             py::bytes object, std::optional<MlirAttribute> mlirObjectProps,
+          [](nb::object cls, MlirAttribute target, uint32_t format,
+             nb::bytes object, std::optional<MlirAttribute> mlirObjectProps,
              std::optional<MlirAttribute> mlirKernelsAttr) {
-            py::buffer_info info(py::buffer(object).request());
-            MlirStringRef objectStrRef =
-                mlirStringRefCreate(static_cast<char *>(info.ptr), info.size);
+            MlirStringRef objectStrRef = mlirStringRefCreate(
+                static_cast<char *>(const_cast<void *>(object.data())),
+                object.size());
             return cls(mlirGPUObjectAttrGetWithKernels(
                 mlirAttributeGetContext(target), target, format, objectStrRef,
                 mlirObjectProps.has_value() ? *mlirObjectProps
@@ -61,7 +63,7 @@ PYBIND11_MODULE(_mlirDialectsGPU, m) {
                                             : MlirAttribute{nullptr}));
           },
           "cls"_a, "target"_a, "format"_a, "object"_a,
-          "properties"_a = py::none(), "kernels"_a = py::none(),
+          "properties"_a.none() = nb::none(), "kernels"_a.none() = nb::none(),
           "Gets a gpu.object from parameters.")
       .def_property_readonly(
           "target",
@@ -73,18 +75,18 @@ PYBIND11_MODULE(_mlirDialectsGPU, m) {
           "object",
           [](MlirAttribute self) {
             MlirStringRef stringRef = mlirGPUObjectAttrGetObject(self);
-            return py::bytes(stringRef.data, stringRef.length);
+            return nb::bytes(stringRef.data, stringRef.length);
           })
       .def_property_readonly("properties",
-                             [](MlirAttribute self) {
+                             [](MlirAttribute self) -> nb::object {
                                if (mlirGPUObjectAttrHasProperties(self))
-                                 return py::cast(
+                                 return nb::cast(
                                      mlirGPUObjectAttrGetProperties(self));
-                               return py::none().cast<py::object>();
+                               return nb::none();
                              })
-      .def_property_readonly("kernels", [](MlirAttribute self) {
+      .def_property_readonly("kernels", [](MlirAttribute self) -> nb::object {
         if (mlirGPUObjectAttrHasKernels(self))
-          return py::cast(mlirGPUObjectAttrGetKernels(self));
-        return py::none().cast<py::object>();
+          return nb::cast(mlirGPUObjectAttrGetKernels(self));
+        return nb::none();
       });
 }
diff --git a/mlir/lib/Bindings/Python/DialectLLVM.cpp b/mlir/lib/Bindings/Python/DialectLLVM.cpp
index cccf1370b8cc87..93714c43e153cf 100644
--- a/mlir/lib/Bindings/Python/DialectLLVM.cpp
+++ b/mlir/lib/Bindings/Python/DialectLLVM.cpp
@@ -12,15 +12,23 @@
 #include "mlir-c/IR.h"
 #include "mlir-c/Support.h"
 #include "mlir/Bindings/Python/Diagnostics.h"
-#include "mlir/Bindings/Python/PybindAdaptors.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
+
+#include <nanobind/nanobind.h>
+#include <nanobind/stl/optional.h>
+#include <nanobind/stl/string.h>
+#include <nanobind/stl/vector.h>
+
+namespace nb = nanobind;
+
+using namespace nanobind::literals;
 
-namespace py = pybind11;
 using namespace llvm;
 using namespace mlir;
 using namespace mlir::python;
-using namespace mlir::python::adaptors;
+using namespace mlir::python::nanobind_adaptors;
 
-void populateDialectLLVMSubmodule(const pybind11::module &m) {
+void populateDialectLLVMSubmodule(const nanobind::module_ &m) {
 
   //===--------------------------------------------------------------------===//
   // StructType
@@ -31,35 +39,35 @@ void populateDialectLLVMSubmodule(const pybind11::module &m) {
 
   llvmStructType.def_classmethod(
       "get_literal",
-      [](py::object cls, const std::vector<MlirType> &elements, bool packed,
+      [](nb::object cls, const std::vector<MlirType> &elements, bool packed,
          MlirLocation loc) {
         CollectDiagnosticsToStringScope scope(mlirLocationGetContext(loc));
 
         MlirType type = mlirLLVMStructTypeLiteralGetChecked(
             loc, elements.size(), elements.data(), packed);
         if (mlirTypeIsNull(type)) {
-          throw py::value_error(scope.takeMessage());
+          throw nb::value_error(scope.takeMessage().c_str());
         }
         return cls(type);
       },
-      "cls"_a, "elements"_a, py::kw_only(), "packed"_a = false,
-      "loc"_a = py::none());
+      "cls"_a, "elements"_a, nb::kw_only(), "packed"_a = false,
+      "loc"_a.none() = nb::none());
 
   llvmStructType.def_classmethod(
       "get_identified",
-      [](py::object cls, const std::string &name, MlirContext context) {
+      [](nb::object cls, const std::string &name, MlirContext context) {
         return cls(mlirLLVMStructTypeIdentifiedGet(
             context, mlirStringRefCreate(name.data(), name.size())));
       },
-      "cls"_a, "name"_a, py::kw_only(), "context"_a = py::none());
+      "cls"_a, "name"_a, nb::kw_only(), "context"_a.none() = nb::none());
 
   llvmStructType.def_classmethod(
       "get_opaque",
-      [](py::object cls, const std::string &name, MlirContext context) {
+      [](nb::object cls, const std::string &name, MlirContext context) {
         return cls(mlirLLVMStructTypeOpaqueGet(
             context, mlirStringRefCreate(name.data(), name.size())));
       },
-      "cls"_a, "name"_a, "context"_a = py::none());
+      "cls"_a, "name"_a, "context"_a.none() = nb::none());
 
   llvmStructType.def(
       "set_body",
@@ -67,22 +75,22 @@ void populateDialectLLVMSubmodule(const pybind11::module &m) {
         MlirLogicalResult result = mlirLLVMStructTypeSetBody(
             self, elements.size(), elements.data(), packed);
         if (!mlirLogicalResultIsSuccess(result)) {
-          throw py::value_error(
+          throw nb::value_error(
               "Struct body already set to different content.");
         }
       },
-      "elements"_a, py::kw_only(), "packed"_a = false);
+      "elements"_a, nb::kw_only(), "packed"_a = false);
 
   llvmStructType.def_classmethod(
       "new_identified",
-      [](py::object cls, const std::string &name,
+      [](nb::object cls, const std::string &name,
          const std::vector<MlirType> &elements, bool packed, MlirContext ctx) {
         return cls(mlirLLVMStructTypeIdentifiedNewGet(
             ctx, mlirStringRefCreate(name.data(), name.length()),
             elements.size(), elements.data(), packed));
       },
-      "cls"_a, "name"_a, "elements"_a, py::kw_only(), "packed"_a = false,
-      "context"_a = py::none());
+      "cls"_a, "name"_a, "elements"_a, nb::kw_only(), "packed"_a = false,
+      "context"_a.none() = nb::none());
 
   llvmStructType.def_property_readonly(
       "name", [](MlirType type) -> std::optional<std::string> {
@@ -93,12 +101,12 @@ void populateDialectLLVMSubmodule(const pybind11::module &m) {
         return StringRef(stringRef.data, stringRef.length).str();
       });
 
-  llvmStructType.def_property_readonly("body", [](MlirType type) -> py::object {
+  llvmStructType.def_property_readonly("body", [](MlirType type) -> nb::object {
     // Don't crash in absence of a body.
     if (mlirLLVMStructTypeIsOpaque(type))
-      return py::none();
+      return nb::none();
 
-    py::list body;
+    nb::list body;
     for (intptr_t i = 0, e = mlirLLVMStructTypeGetNumElementTypes(type); i < e;
          ++i) {
       body.append(mlirLLVMStructTypeGetElementType(type, i));
@@ -119,24 +127,24 @@ void populateDialectLLVMSubmodule(const pybind11::module &m) {
   mlir_type_subclass(m, "PointerType", mlirTypeIsALLVMPointerType)
       .def_classmethod(
           "get",
-          [](py::object cls, std::optional<unsigned> addressSpace,
+          [](nb::object cls, std::optional<unsigned> addressSpace,
              MlirContext context) {
             CollectDiagnosticsToStringScope scope(context);
             MlirType type = mlirLLVMPointerTypeGet(
                 context, addressSpace.has_value() ? *addressSpace : 0);
             if (mlirTypeIsNull(type)) {
-              throw py::value_error(scope.takeMessage());
+              throw nb::value_error(scope.takeMessage().c_str());
             }
             return cls(type);
           },
-          "cls"_a, "address_space"_a = py::none(), py::kw_only(),
-          "context"_a = py::none())
+          "cls"_a, "address_space"_a.none() = nb::none(), nb::kw_only(),
+          "context"_a.none() = nb::none())
       .def_property_readonly("address_space", [](MlirType type) {
         return mlirLLVMPointerTypeGetAddressSpace(type);
       });
 }
 
-PYBIND11_MODULE(_mlirDialectsLLVM, m) {
+NB_MODULE(_mlirDialectsLLVM, m) {
   m.doc() = "MLIR LLVM Dialect";
 
   populateDialectLLVMSubmodule(m);
diff --git a/mlir/lib/Bindings/Python/DialectLinalg.cpp b/mlir/lib/Bindings/Python/DialectLinalg.cpp
index 2e54ebeb61fb10..4ac031ba2b145b 100644
--- a/mlir/lib/Bindings/Python/DialectLinalg.cpp
+++ b/mlir/lib/Bindings/Python/DialectLinalg.cpp
@@ -6,22 +6,24 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include <nanobind/nanobind.h>
+
 #include "mlir-c/Dialect/Linalg.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/PybindAdaptors.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
-namespace py = pybind11;
+namespace nb = nanobind;
 
-static void populateDialectLinalgSubmodule(py::module m) {
+static void populateDialectLinalgSubmodule(nb::module_ m) {
   m.def(
       "fill_builtin_region",
       [](MlirOperation op) { mlirLinalgFillBuiltinNamedOpRegion(op); },
-      py::arg("op"),
+      nb::arg("op"),
       "Fill the region for `op`, which is assumed to be a builtin named Linalg "
       "op.");
 }
 
-PYBIND11_MODULE(_mlirDialectsLinalg, m) {
+NB_MODULE(_mlirDialectsLinalg, m) {
   m.doc() = "MLIR Linalg dialect.";
 
   populateDialectLinalgSubmodule(m);
diff --git a/mlir/lib/Bindings/Python/DialectNVGPU.cpp b/mlir/lib/Bindings/Python/DialectNVGPU.cpp
index 754e0a75b0abc7..6937c686277cad 100644
--- a/mlir/lib/Bindings/Python/DialectNVGPU.cpp
+++ b/mlir/lib/Bindings/Python/DialectNVGPU.cpp
@@ -6,35 +6,36 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include <nanobind/nanobind.h>
+
 #include "mlir-c/Dialect/NVGPU.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/PybindAdaptors.h"
-#include <pybind11/pybind11.h>
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
-namespace py = pybind11;
+namespace nb = nanobind;
 using namespace llvm;
 using namespace mlir;
 using namespace mlir::python;
-using namespace mlir::python::adaptors;
+using namespace mlir::python::nanobind_adaptors;
 
-static void populateDialectNVGPUSubmodule(const pybind11::module &m) {
+static void populateDialectNVGPUSubmodule(const nb::module_ &m) {
   auto nvgpuTensorMapDescriptorType = mlir_type_subclass(
       m, "TensorMapDescriptorType", mlirTypeIsANVGPUTensorMapDescriptorType);
 
   nvgpuTensorMapDescriptorType.def_classmethod(
       "get",
-      [](py::object cls, MlirType tensorMemrefType, int swizzle, int l2promo,
+      [](nb::object cls, MlirType tensorMemrefType, int swizzle, int l2promo,
          int oobFill, int interleave, MlirContext ctx) {
         return cls(mlirNVGPUTensorMapDescriptorTypeGet(
             ctx, tensorMemrefType, swizzle, l2promo, oobFill, interleave));
       },
       "Gets an instance of TensorMapDescriptorType in the same context",
-      py::arg("cls"), py::arg("tensor_type"), py::arg("swizzle"),
-      py::arg("l2promo"), py::arg("oob_fill"), py::arg("interleave"),
-      py::arg("ctx") = py::none());
+      nb::arg("cls"), nb::arg("tensor_type"), nb::arg("swizzle"),
+      nb::arg("l2promo"), nb::arg("oob_fill"), nb::arg("interleave"),
+      nb::arg("ctx").none() = nb::none());
 }
 
-PYBIND11_MODULE(_mlirDialectsNVGPU, m) {
+NB_MODULE(_mlirDialectsNVGPU, m) {
   m.doc() = "MLIR NVGPU dialect.";
 
   populateDialectNVGPUSubmodule(m);
diff --git a/mlir/lib/Bindings/Python/DialectPDL.cpp b/mlir/lib/Bindings/Python/DialectPDL.cpp
index 8d3f9a7ab1d6ac..56a7968c06384a 100644
--- a/mlir/lib/Bindings/Python/DialectPDL.cpp
+++ b/mlir/lib/Bindings/Python/DialectPDL.cpp
@@ -6,21 +6,19 @@
 //
 //===----------------------------------------------------------------------===//
 
+#include <nanobind/nanobind.h>
+
 #include "mlir-c/Dialect/PDL.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/PybindAdaptors.h"
-#include <pybind11/cast.h>
-#include <pybind11/detail/common.h>
-#include <pybind11/pybind11.h>
-#include <pybind11/pytypes.h>
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
-namespace py = pybind11;
+namespace nb = nanobind;
 using namespace llvm;
 using namespace mlir;
 using namespace mlir::python;
-using namespace mlir::python::adaptors;
+using namespace mlir::python::nanobind_adaptors;
 
-void populateDialectPDLSubmodule(const pybind11::module &m) {
+void populateDialectPDLSubmodule(const nanobind::module_ &m) {
   //===-------------------------------------------------------------------===//
   // PDLType
   //===-------------------------------------------------------------------===//
@@ -35,11 +33,11 @@ void populateDialectPDLSubmodule(const pybind11::module &m) {
       mlir_type_subclass(m, "AttributeType", mlirTypeIsAPDLAttributeType);
   attributeType.def_classmethod(
       "get",
-      [](py::object cls, MlirContext ctx) {
+      [](nb::object cls, MlirContext ctx) {
         return cls(mlirPDLAttributeTypeGet(ctx));
       },
-      "Get an instance of AttributeType in given context.", py::arg("cls"),
-      py::arg("context") = py::none());
+      "Get an instance of AttributeType in given context.", nb::arg("cls"),
+      nb::arg("context").none() = nb::none());
 
   //===-------------------------------------------------------------------===//
   // OperationType
@@ -49,11 +47,11 @@ void populateDialectPDLSubmodule(const pybind11::module &m) {
       mlir_type_subclass(m, "OperationType", mlirTypeIsAPDLOperationType);
   operationType.def_classmethod(
       "get",
-      [](py::object cls, MlirContext ctx) {
+      [](nb::object cls, MlirContext ctx) {
         return cls(mlirPDLOperationTypeGet(ctx));
       },
-      "Get an instance of OperationType in given context.", py::arg("cls"),
-      py::arg("context") = py::none());
+      "Get an instance of OperationType in given context.", nb::arg("cls"),
+      nb::arg("context").none() = nb::none());
 
   //===-------------------------------------------------------------------===//
   // RangeType
@@ -62,12 +60,12 @@ void populateDialectPDLSubmodule(const pybind11::module &m) {
   auto rangeType = mlir_type_subclass(m, "RangeType", mlirTypeIsAPDLRangeType);
   rangeType.def_classmethod(
       "get",
-      [](py::object cls, MlirType elementType) {
+      [](nb::object cls, MlirType elementType) {
         return cls(mlirPDLRangeTypeGet(elementType));
       },
       "Gets an instance of RangeType in the same context as the provided "
       "element type.",
-      py::arg("cls"), py::arg("element_type"));
+      nb::arg("cls"), nb::arg("element_type"));
   rangeType.def_property_readonly(
       "element_type",
       [](MlirType type) { return mlirPDLRangeTypeGetElementType(type); },
@@ -80,11 +78,11 @@ void populateDialectPDLSubmodule(const pybind11::module &m) {
   auto typeType = mlir_type_subclass(m, "TypeType", mlirTypeIsAPDLTypeType);
   typeType.def_classmethod(
       "get",
-      [](py::object cls, MlirContext ctx) {
+      [](nb::object cls, MlirContext ctx) {
         return cls(mlirPDLTypeTypeGet(ctx));
       },
-      "Get an instance of TypeType in given context.", py::arg("cls"),
-      py::arg("context") = py::none());
+      "Get an instance of TypeType in given context.",...
[truncated]

@hawkinsp hawkinsp force-pushed the nb3 branch 3 times, most recently from 69d1d41 to 6e7fd74 Compare December 19, 2024 16:03
@hawkinsp
Copy link
Contributor Author

Rebased this PR on top of main.

I might let #120473 sit for a day or two before merging this, but this PR should be good to go.

Copy link
Member

@jpienaar jpienaar left a comment

Choose a reason for hiding this comment

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

Overall looks good, I think we can do the following to avoid more of the nanobind warnings (else it just results in too much logging)

diff --git a/mlir/cmake/modules/AddMLIRPython.cmake b/mlir/cmake/modules/AddMLIRPython.cmake
index 67619a90c90b..e5952d1873ef 100644
--- a/mlir/cmake/modules/AddMLIRPython.cmake
+++ b/mlir/cmake/modules/AddMLIRPython.cmake
@@ -142,6 +142,9 @@ function(declare_mlir_python_extension name)
     mlir_python_DEPENDS ""
     mlir_python_BINDINGS_LIBRARY "${ARG_PYTHON_BINDINGS_LIBRARY}"
   )
+  if (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL)
+    set_target_properties(${name} PROPERTIES INTERFACE_COMPILE_OPTIONS "-Wno-cast-qual;-Wno-zero-length-array;-Wno-extra-semi;-Wno-nested-anon-types;-Wno-pedantic")
+  endif()

   # Set the interface source and link_libs properties of the target
   # These properties support generator expressions and are automatically exported
diff --git a/mlir/python/CMakeLists.txt b/mlir/python/CMakeLists.txt
index 4f3a4e67d6f1..84b43a70ec61 100644
--- a/mlir/python/CMakeLists.txt
+++ b/mlir/python/CMakeLists.txt
@@ -476,9 +476,6 @@ declare_mlir_python_extension(MLIRPythonExtension.Core
     # Dialects
     MLIRCAPIFunc
 )
-if (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL)
-set_target_properties(MLIRPythonExtension.Core PROPERTIES INTERFACE_COMPILE_OPTIONS "-Wno-cast-qual;-Wno-zero-length-array;-Wno-extra-semi;-Wno-nested-anon-types;-Wno-pedantic")
-endif()

 # This extension exposes an API to register all dialects, extensions, and passes
 # packaged in upstream MLIR and it is used for the upstream "mlir" Python
@@ -750,9 +747,6 @@ if(MLIR_INCLUDE_TESTS)
     EMBED_CAPI_LINK_LIBS
       MLIRCAPIPythonTestDialect
   )
-  if (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL)
-    set_target_properties(MLIRPythonTestSources.PythonTestExtensionNanobind PROPERTIES INTERFACE_COMPILE_OPTIONS "-Wno-cast-qual;-Wno-zero-length-array;-Wno-extra-semi;-Wno-nested-anon-types;-Wno-pedantic")
-  endif()
 endif()

@hawkinsp
Copy link
Contributor Author

Overall looks good, I think we can do the following to avoid more of the nanobind warnings (else it just results in too much logging)
...

Done

@joker-eph
Copy link
Collaborator

Overall looks good, I think we can do the following to avoid more of the nanobind warnings (else it just results in too much logging)

Instead of disabling warnings when building our sources, can we just disable them on nanobing headers? (that is defining the nanobind include path as system headers instead of regular include)

@hawkinsp
Copy link
Contributor Author

Overall looks good, I think we can do the following to avoid more of the nanobind warnings (else it just results in too much logging)

Instead of disabling warnings when building our sources, can we just disable them on nanobing headers? (that is defining the nanobind include path as system headers instead of regular include)

How does one do that, exactly?

@makslevental
Copy link
Contributor

makslevental commented Dec 20, 2024

How does one do that, exactly?

This isn't what Mehdi is suggesting but you can do this around the headers I think (push/pop):

https://gcc.gnu.org/onlinedocs/gcc/Diagnostic-Pragmas.html

Clang has the same thing but I can't find the docs right this moment

@joker-eph
Copy link
Collaborator

In CMake it is the SYSTEM option of include_directories.
Since you're including nanobind using find_package, I think you don't have control on this. But you can remap it after find_package in the macro we use to inject the nanobind dependency like this:

# Retrieve the include directories set by nanobind
get_target_property(nanobind_includes nanobind::nanobind INTERFACE_INCLUDE_DIRECTORIES)
# Apply them as SYSTEM include directories
target_include_directories(my_target SYSTEM PRIVATE ${nanobind_includes})

@hawkinsp
Copy link
Contributor Author

In CMake it is the SYSTEM option of include_directories. Since you're including nanobind using find_package, I think you don't have control on this. But you can remap it after find_package in the macro we use to inject the nanobind dependency like this:

# Retrieve the include directories set by nanobind
get_target_property(nanobind_includes nanobind::nanobind INTERFACE_INCLUDE_DIRECTORIES)
# Apply them as SYSTEM include directories
target_include_directories(my_target SYSTEM PRIVATE ${nanobind_includes})

I spent 20 minutes trying to do this and failing. But I admit CMake is completely incomprehensible to me. Is this a hard blocker to landing it?

@makslevental
Copy link
Contributor

I spent 20 minutes trying to do this and failing. But I admit CMake is completely incomprehensible to me. Is this a hard blocker to landing it?

Try with quotes like

target_include_directories(my_target SYSTEM PRIVATE "${nanobind_includes}")

CMake is very particular about what's a list/arg/string

@jpienaar
Copy link
Member

Its weird, with target_include_directories set, it affects the standalone example's python. It looks like the discovery of _root_dir.

@makslevental
Copy link
Contributor

Its weird, with target_include_directories set, it affects the standalone example's python. It looks like the discovery of _root_dir.

It's because INTERFACE in INTERFACE_INCLUDE_DIRECTORIES means "propagate" (roughly I guess).

So it occurs to me, do we actually need INTERFACE here

+  if (LLVM_COMPILER_IS_GCC_COMPATIBLE OR CLANG_CL)
+    set_target_properties(${name} PROPERTIES INTERFACE_COMPILE_OPTIONS "-Wno-cast-qual;-Wno-zero-length-array;-Wno-extra-semi;-Wno-nested-anon-types;-Wno-pedantic")
+  endif()

@jpienaar
Copy link
Member

I thought I removed those in the switch to system headers. But yes they were needed (at least cmake complained if I used private). But my cmake knowledge is of such a level that I should probably just ask Gemini for a solution :)

@makslevental
Copy link
Contributor

makslevental commented Dec 20, 2024

I got it - you need to fiddle with nanobind's library target itself https://github.com/wjakob/nanobind/blob/b7c4f1abfcab9cc5a8f0ef758926d92ff5eac3a3/cmake/nanobind-config.cmake#L358

which for us would look like

target_compile_options(nanobind-static
  PRIVATE
  -Wno-cast-qual
  -Wno-zero-length-array
 ...
)

note that target name is probably an "implementation detail" (you can see just above that line the various args to nanobind_module can affect the target name) but 🤷

Copy link

github-actions bot commented Dec 20, 2024

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff 44514316bd5ef656076b6baaf6bccb298d98f0ea c233322b29e4919087553ff0fbd88e2a27309928 --extensions h,cpp -- mlir/include/mlir/Bindings/Python/Nanobind.h mlir/examples/standalone/python/StandaloneExtensionNanobind.cpp mlir/include/mlir/Bindings/Python/NanobindAdaptors.h mlir/lib/Bindings/Python/AsyncPasses.cpp mlir/lib/Bindings/Python/DialectGPU.cpp mlir/lib/Bindings/Python/DialectLLVM.cpp mlir/lib/Bindings/Python/DialectLinalg.cpp mlir/lib/Bindings/Python/DialectNVGPU.cpp mlir/lib/Bindings/Python/DialectPDL.cpp mlir/lib/Bindings/Python/DialectQuant.cpp mlir/lib/Bindings/Python/DialectSparseTensor.cpp mlir/lib/Bindings/Python/DialectTransform.cpp mlir/lib/Bindings/Python/ExecutionEngineModule.cpp mlir/lib/Bindings/Python/GPUPasses.cpp mlir/lib/Bindings/Python/IRAffine.cpp mlir/lib/Bindings/Python/IRAttributes.cpp mlir/lib/Bindings/Python/IRCore.cpp mlir/lib/Bindings/Python/IRInterfaces.cpp mlir/lib/Bindings/Python/IRModule.cpp mlir/lib/Bindings/Python/IRModule.h mlir/lib/Bindings/Python/IRTypes.cpp mlir/lib/Bindings/Python/LinalgPasses.cpp mlir/lib/Bindings/Python/MainModule.cpp mlir/lib/Bindings/Python/NanobindUtils.h mlir/lib/Bindings/Python/Pass.cpp mlir/lib/Bindings/Python/RegisterEverything.cpp mlir/lib/Bindings/Python/Rewrite.cpp mlir/lib/Bindings/Python/SparseTensorPasses.cpp mlir/lib/Bindings/Python/TransformInterpreter.cpp mlir/test/python/lib/PythonTestModuleNanobind.cpp
View the diff from clang-format here.
diff --git a/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h b/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
index 517351cac6..3223ae9dce 100644
--- a/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
+++ b/mlir/include/mlir/Bindings/Python/NanobindAdaptors.h
@@ -21,10 +21,10 @@
 
 #include <cstdint>
 
+#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir-c/Diagnostics.h"
 #include "mlir-c/IR.h"
 #include "mlir/Bindings/Python/Nanobind.h"
-#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "llvm/ADT/Twine.h"
 
 // Raw CAPI type casters need to be declared before use, so always include them
diff --git a/mlir/lib/Bindings/Python/DialectGPU.cpp b/mlir/lib/Bindings/Python/DialectGPU.cpp
index e5045cf0bb..29a313f6da 100644
--- a/mlir/lib/Bindings/Python/DialectGPU.cpp
+++ b/mlir/lib/Bindings/Python/DialectGPU.cpp
@@ -9,8 +9,8 @@
 #include "mlir-c/Dialect/GPU.h"
 #include "mlir-c/IR.h"
 #include "mlir-c/Support.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace nanobind::literals;
diff --git a/mlir/lib/Bindings/Python/DialectLLVM.cpp b/mlir/lib/Bindings/Python/DialectLLVM.cpp
index f211e769d6..a4d8aa41bf 100644
--- a/mlir/lib/Bindings/Python/DialectLLVM.cpp
+++ b/mlir/lib/Bindings/Python/DialectLLVM.cpp
@@ -12,8 +12,8 @@
 #include "mlir-c/IR.h"
 #include "mlir-c/Support.h"
 #include "mlir/Bindings/Python/Diagnostics.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 
diff --git a/mlir/lib/Bindings/Python/DialectLinalg.cpp b/mlir/lib/Bindings/Python/DialectLinalg.cpp
index 548df4ee10..0f4d016443 100644
--- a/mlir/lib/Bindings/Python/DialectLinalg.cpp
+++ b/mlir/lib/Bindings/Python/DialectLinalg.cpp
@@ -8,8 +8,8 @@
 
 #include "mlir-c/Dialect/Linalg.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 
diff --git a/mlir/lib/Bindings/Python/DialectNVGPU.cpp b/mlir/lib/Bindings/Python/DialectNVGPU.cpp
index a0d6a4b4c7..bd4a9521ba 100644
--- a/mlir/lib/Bindings/Python/DialectNVGPU.cpp
+++ b/mlir/lib/Bindings/Python/DialectNVGPU.cpp
@@ -8,8 +8,8 @@
 
 #include "mlir-c/Dialect/NVGPU.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace llvm;
diff --git a/mlir/lib/Bindings/Python/DialectPDL.cpp b/mlir/lib/Bindings/Python/DialectPDL.cpp
index bcc6ff406c..d0dcd613e1 100644
--- a/mlir/lib/Bindings/Python/DialectPDL.cpp
+++ b/mlir/lib/Bindings/Python/DialectPDL.cpp
@@ -8,8 +8,8 @@
 
 #include "mlir-c/Dialect/PDL.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace llvm;
diff --git a/mlir/lib/Bindings/Python/DialectQuant.cpp b/mlir/lib/Bindings/Python/DialectQuant.cpp
index 29f19c9c50..632b7d140e 100644
--- a/mlir/lib/Bindings/Python/DialectQuant.cpp
+++ b/mlir/lib/Bindings/Python/DialectQuant.cpp
@@ -11,8 +11,8 @@
 
 #include "mlir-c/Dialect/Quant.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace llvm;
diff --git a/mlir/lib/Bindings/Python/DialectSparseTensor.cpp b/mlir/lib/Bindings/Python/DialectSparseTensor.cpp
index 97cebcceeb..27b731b5bd 100644
--- a/mlir/lib/Bindings/Python/DialectSparseTensor.cpp
+++ b/mlir/lib/Bindings/Python/DialectSparseTensor.cpp
@@ -12,8 +12,8 @@
 #include "mlir-c/AffineMap.h"
 #include "mlir-c/Dialect/SparseTensor.h"
 #include "mlir-c/IR.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace llvm;
diff --git a/mlir/lib/Bindings/Python/DialectTransform.cpp b/mlir/lib/Bindings/Python/DialectTransform.cpp
index 59a030ac67..0190edf791 100644
--- a/mlir/lib/Bindings/Python/DialectTransform.cpp
+++ b/mlir/lib/Bindings/Python/DialectTransform.cpp
@@ -11,8 +11,8 @@
 #include "mlir-c/Dialect/Transform.h"
 #include "mlir-c/IR.h"
 #include "mlir-c/Support.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace mlir;
diff --git a/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp b/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
index 81dada3553..b4c852c2de 100644
--- a/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
+++ b/mlir/lib/Bindings/Python/ExecutionEngineModule.cpp
@@ -7,8 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "mlir-c/ExecutionEngine.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 using namespace mlir;
diff --git a/mlir/lib/Bindings/Python/IRAffine.cpp b/mlir/lib/Bindings/Python/IRAffine.cpp
index a2df824f59..80b203d2c5 100644
--- a/mlir/lib/Bindings/Python/IRAffine.cpp
+++ b/mlir/lib/Bindings/Python/IRAffine.cpp
@@ -17,9 +17,9 @@
 #include "NanobindUtils.h"
 #include "mlir-c/AffineExpr.h"
 #include "mlir-c/AffineMap.h"
+#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir-c/IntegerSet.h"
 #include "mlir/Bindings/Python/Nanobind.h"
-#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir/Support/LLVM.h"
 #include "llvm/ADT/Hashing.h"
 #include "llvm/ADT/SmallVector.h"
diff --git a/mlir/lib/Bindings/Python/IRAttributes.cpp b/mlir/lib/Bindings/Python/IRAttributes.cpp
index 08f7d4881e..85b4beef7f 100644
--- a/mlir/lib/Bindings/Python/IRAttributes.cpp
+++ b/mlir/lib/Bindings/Python/IRAttributes.cpp
@@ -16,8 +16,8 @@
 #include "NanobindUtils.h"
 #include "mlir-c/BuiltinAttributes.h"
 #include "mlir-c/BuiltinTypes.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "llvm/ADT/ScopeExit.h"
 #include "llvm/Support/raw_ostream.h"
 
diff --git a/mlir/lib/Bindings/Python/IRCore.cpp b/mlir/lib/Bindings/Python/IRCore.cpp
index 86afa95639..fca1b4d344 100644
--- a/mlir/lib/Bindings/Python/IRCore.cpp
+++ b/mlir/lib/Bindings/Python/IRCore.cpp
@@ -12,6 +12,7 @@
 #include "Globals.h"
 #include "IRModule.h"
 #include "NanobindUtils.h"
+#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir-c/BuiltinAttributes.h"
 #include "mlir-c/Debug.h"
 #include "mlir-c/Diagnostics.h"
@@ -19,7 +20,6 @@
 #include "mlir-c/Support.h"
 #include "mlir/Bindings/Python/Nanobind.h"
 #include "mlir/Bindings/Python/NanobindAdaptors.h"
-#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
 
diff --git a/mlir/lib/Bindings/Python/IRModule.cpp b/mlir/lib/Bindings/Python/IRModule.cpp
index f7bf77e5a7..18fe417d10 100644
--- a/mlir/lib/Bindings/Python/IRModule.cpp
+++ b/mlir/lib/Bindings/Python/IRModule.cpp
@@ -13,9 +13,9 @@
 
 #include "Globals.h"
 #include "NanobindUtils.h"
+#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir-c/Support.h"
 #include "mlir/Bindings/Python/Nanobind.h"
-#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 
 namespace nb = nanobind;
 using namespace mlir;
diff --git a/mlir/lib/Bindings/Python/IRModule.h b/mlir/lib/Bindings/Python/IRModule.h
index 8fb32a225e..8996e929d7 100644
--- a/mlir/lib/Bindings/Python/IRModule.h
+++ b/mlir/lib/Bindings/Python/IRModule.h
@@ -22,8 +22,8 @@
 #include "mlir-c/IR.h"
 #include "mlir-c/IntegerSet.h"
 #include "mlir-c/Transforms.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "llvm/ADT/DenseMap.h"
 
 namespace mlir {
diff --git a/mlir/lib/Bindings/Python/MainModule.cpp b/mlir/lib/Bindings/Python/MainModule.cpp
index 7c40642620..97e5bb7fca 100644
--- a/mlir/lib/Bindings/Python/MainModule.cpp
+++ b/mlir/lib/Bindings/Python/MainModule.cpp
@@ -6,7 +6,6 @@
 //
 //===----------------------------------------------------------------------===//
 
-
 #include "Globals.h"
 #include "IRModule.h"
 #include "NanobindUtils.h"
diff --git a/mlir/lib/Bindings/Python/Pass.cpp b/mlir/lib/Bindings/Python/Pass.cpp
index 858c3bd574..7dd9866457 100644
--- a/mlir/lib/Bindings/Python/Pass.cpp
+++ b/mlir/lib/Bindings/Python/Pass.cpp
@@ -9,9 +9,9 @@
 #include "Pass.h"
 
 #include "IRModule.h"
+#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir-c/Pass.h"
 #include "mlir/Bindings/Python/Nanobind.h"
-#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 
 namespace nb = nanobind;
 using namespace nb::literals;
diff --git a/mlir/lib/Bindings/Python/RegisterEverything.cpp b/mlir/lib/Bindings/Python/RegisterEverything.cpp
index 3ba42bec5d..3edcb099c0 100644
--- a/mlir/lib/Bindings/Python/RegisterEverything.cpp
+++ b/mlir/lib/Bindings/Python/RegisterEverything.cpp
@@ -7,8 +7,8 @@
 //===----------------------------------------------------------------------===//
 
 #include "mlir-c/RegisterEverything.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 NB_MODULE(_mlirRegisterEverything, m) {
   m.doc() = "MLIR All Upstream Dialects, Translations and Passes Registration";
diff --git a/mlir/lib/Bindings/Python/Rewrite.cpp b/mlir/lib/Bindings/Python/Rewrite.cpp
index 0373f9c7af..eb5c493625 100644
--- a/mlir/lib/Bindings/Python/Rewrite.cpp
+++ b/mlir/lib/Bindings/Python/Rewrite.cpp
@@ -9,9 +9,9 @@
 #include "Rewrite.h"
 
 #include "IRModule.h"
+#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir-c/Rewrite.h"
 #include "mlir/Bindings/Python/Nanobind.h"
-#include "mlir-c/Bindings/Python/Interop.h" // This is expected after nanobind.
 #include "mlir/Config/mlir-config.h"
 
 namespace nb = nanobind;
diff --git a/mlir/lib/Bindings/Python/TransformInterpreter.cpp b/mlir/lib/Bindings/Python/TransformInterpreter.cpp
index f9b0fed627..8d1abfb9af 100644
--- a/mlir/lib/Bindings/Python/TransformInterpreter.cpp
+++ b/mlir/lib/Bindings/Python/TransformInterpreter.cpp
@@ -14,8 +14,8 @@
 #include "mlir-c/IR.h"
 #include "mlir-c/Support.h"
 #include "mlir/Bindings/Python/Diagnostics.h"
-#include "mlir/Bindings/Python/NanobindAdaptors.h"
 #include "mlir/Bindings/Python/Nanobind.h"
+#include "mlir/Bindings/Python/NanobindAdaptors.h"
 
 namespace nb = nanobind;
 

Copy link
Collaborator

@joker-eph joker-eph left a comment

Choose a reason for hiding this comment

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

LG!

This is a companion to llvm#118583, although it can be landed independently
because since llvm#117922 dialects do not have to use the same Python
binding framework as the Python core code.

This PR ports all of the in-tree dialect and pass extensions to nanobind,
with the exception of those that remain for testing pybind11 support. It
would make sense to merge this PR after merging llvm#118583, if we have
agreed that we are migrating the core to nanobind.

This PR also:
* removes CollectDiagnosticsToStringScope from NanobindAdaptors.h. This
  was overlooked in a previous PR and it is duplicated in Diagnostics.h.

---------

Co-authored-by: Jacques Pienaar <[email protected]>
Comment in Interop.h saying expectation on Windows is including this
after pybind11 - going to try the same for nanobind.
@jpienaar jpienaar merged commit 5cd4274 into llvm:main Dec 21, 2024
7 of 8 checks passed
@makslevental
Copy link
Contributor

FYI after upgrading mlir-python-extras to latest I am suddenly getting this:

nanobind: leaked 3 instances!
 - leaked instance 0x7fe74e5c1c88 of type "mlir.execution_engine.ExecutionEngine"
 - leaked instance 0x7fe74e5ba868 of type "mlir._mlir_libs._mlir.ir.UnrankedMemRefType"
 - leaked instance 0x7fe74e67afa8 of type "mlir._mlir_libs.Context"
nanobind: leaked 126 types!
 - leaked type "mlir._mlir_libs._mlir.ir.AffineFloorDivExpr"
 - leaked type "mlir._mlir_libs._mlir.ir.AffineCeilDivExpr"
 - leaked type "mlir._mlir_libs._mlir.ir.F16Type"
 - leaked type "mlir._mlir_libs._mlir.ir.AffineMap"
 - leaked type "mlir._mlir_libs._mlirExecutionEngine.ExecutionEngine"
 - leaked type "mlir._mlir_libs._mlir.ir.FloatTF32Type"
 - leaked type "mlir._mlir_libs._mlir.ir.Float8E8M0FNUType"
 - leaked type "mlir._mlir_libs._mlir.ir.IntegerAttr"
 - leaked type "mlir._mlir_libs._mlir.ir.OpOperandIterator"
 - leaked type "mlir._mlir_libs._mlir.ir.OpView"
 - leaked type "mlir._mlir_libs._mlir.passmanager.PassManager"
 - ... skipped remainder
nanobind: leaked 860 functions!
 - leaked function "__init__"
 - leaked function ""
 - leaked function ""
 - leaked function ""
 - leaked function "dump"
 - leaked function "get_identity"
 - leaked function "__add__"
 - leaked function "maybe_downcast"
 - leaked function "__init__"
 - leaked function ""
 - leaked function ""
 - ... skipped remainder
nanobind: this is likely caused by a reference counting issue in the binding code.

I'm not sure if this is my fault or upstream's 🤷

@stellaraccident
Copy link
Contributor

FYI after upgrading mlir-python-extras to latest I am suddenly getting this:

nanobind: leaked 3 instances!
 - leaked instance 0x7fe74e5c1c88 of type "mlir.execution_engine.ExecutionEngine"
 - leaked instance 0x7fe74e5ba868 of type "mlir._mlir_libs._mlir.ir.UnrankedMemRefType"
 - leaked instance 0x7fe74e67afa8 of type "mlir._mlir_libs.Context"
nanobind: leaked 126 types!
 - leaked type "mlir._mlir_libs._mlir.ir.AffineFloorDivExpr"
 - leaked type "mlir._mlir_libs._mlir.ir.AffineCeilDivExpr"
 - leaked type "mlir._mlir_libs._mlir.ir.F16Type"
 - leaked type "mlir._mlir_libs._mlir.ir.AffineMap"
 - leaked type "mlir._mlir_libs._mlirExecutionEngine.ExecutionEngine"
 - leaked type "mlir._mlir_libs._mlir.ir.FloatTF32Type"
 - leaked type "mlir._mlir_libs._mlir.ir.Float8E8M0FNUType"
 - leaked type "mlir._mlir_libs._mlir.ir.IntegerAttr"
 - leaked type "mlir._mlir_libs._mlir.ir.OpOperandIterator"
 - leaked type "mlir._mlir_libs._mlir.ir.OpView"
 - leaked type "mlir._mlir_libs._mlir.passmanager.PassManager"
 - ... skipped remainder
nanobind: leaked 860 functions!
 - leaked function "__init__"
 - leaked function ""
 - leaked function ""
 - leaked function ""
 - leaked function "dump"
 - leaked function "get_identity"
 - leaked function "__add__"
 - leaked function "maybe_downcast"
 - leaked function "__init__"
 - leaked function ""
 - leaked function ""
 - ... skipped remainder
nanobind: this is likely caused by a reference counting issue in the binding code.

I'm not sure if this is my fault or upstream's 🤷

I've generally found the nanobind leak detector to make false positive warnings in a variety of situations that are common in these kind of programs. I can't tell if this is one of them. There is a function you can call at module init or after to silence it. I'd recommend landing that upstream while investigating.

TBH - the only reliable way I've found to do leak detection is via a python debug build (which explicitly frees at shutdown) and ASAN. Anything else is easily thwarted with valid programs.

marbre added a commit to iree-org/llvm-project that referenced this pull request Jan 5, 2025
MaheshRavishankar added a commit to iree-org/llvm-project that referenced this pull request Jan 7, 2025
MaheshRavishankar added a commit to iree-org/llvm-project that referenced this pull request Jan 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bazel "Peripheral" support tier build system: utils/bazel mlir:python MLIR Python bindings mlir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants