[mlir][Python] Sync Python bindings with C API MlirStringRef modification.

[stellaraccident]

Summary

MLIR C API use the MlirStringRef instead of const char * for the string type now. This patch sync the Python bindings with the C API modification.

[ftynse]

mlir/lib/Bindings/Python/IRModules.cpp

911

Let's use mlirStringRefCreate instead and get the length of the string from std::string. This will avoid the need to list all characters in the string until \0. Bonus points for adding a helper.

[zhanghb97]

mlir/lib/Bindings/Python/IRModules.cpp

911

When I add a helper function like this:

static MlirStringRef createMlirStringRef(std::string str) {
  return mlirStringRefCreate(str.c_str(), str.length());
}

It seems that the lifetime of the std::string isn't extended out of the function. I think I need help here: how can I extend the lifetime of the string, and avoid memory leak at the same time?

[stellaraccident]

Does anyone mind if I commandeer this change (it is broken at head)?

[GeorgeLyon]

Small process question: I authored the change to us MlirStringRef instead of const char *, what is the policy about breaking changes in the C API? I think it is likely unreasonable to expect any change in the C API to fix all in-tree language bindings, but since the Python bindings are in-tree, these types of changes can break them. I'm might be the only person working on non-Python bindings here, so I may be biased :)

[stellaraccident]

In D92007#2414212, @GeorgeLyon wrote:

Small process question: I authored the change to us MlirStringRef instead of const char *, what is the policy about breaking changes in the C API? I think it is likely unreasonable to expect any change in the C API to fix all in-tree language bindings, but since the Python bindings are in-tree, these types of changes can break them. I'm might be the only person working on non-Python bindings here, so I may be biased :)

The RFC where we started the Python bindings identified this as an optional feature, so you're good :) We're working on getting buildbot coverage and making the case to enable it by default, but that will be a discussion. Until then, keeping it updated is best effort.

Also, as a non-trivial user of the C-API in tree, it may eventually be a good buffer against unintended API changes (we want the C-API to be fairly stable).

[stellaraccident]

Commandeer change and resolve comments.

[mehdi_amini]

I think it is likely unreasonable to expect any change in the C API to fix all in-tree language bindings,

Why is that? We update all the C++ API users when we update it and the surface is much higher.

[GeorgeLyon]

The difference is that all the consumers of the C++ API are C++, so it is pretty straightforward to update when making breaking changes even if the surface is greater. Language bindings will live at least partially in their own ecosystem and requiring folks who work on the C API to understand each individual ecosystem may be too onerous, even if the surface area is smaller. To be clear, I’m just saying there is a tradeoff here, not advocating for us to decide on a particular point on that spectrum. I agree with Stella that we can wait until this becomes a bigger issue and then have a more formal discussion (hopefully with more data in the form of diverse language bindings).

[mehdi_amini]

In D92007#2414495, @GeorgeLyon wrote:

The difference is that all the consumers of the C++ API are C++, so it is pretty straightforward to update when making breaking changes even if the surface is greater.

You're clearly not updating all of the Google internal users ;)

Language bindings will live at least partially in their own ecosystem and requiring folks who work on the C API to understand each individual ecosystem may be too onerous, even if the surface area is smaller.

There are a few aspects here that make me doubtful about it:

• the C API is more limited (by the virtue of using C: no overloading sets, no inheritance/overrides, no templates, etc.)
• the C API is intended to be stable at some point and see little churn.
• I does not seem hard to me to see myself changing the C API like here and adjust any in-tree language bindings in a fairly straightforward way, in most cases it'll be fairly mechanical to look at the API call site and update it. I don't quite get why this would be more complex than all of the C++ uses actually.

[mehdi_amini]

mlir/lib/Bindings/Python/IRModules.cpp

911

You should take the string as const std::string &std in the helper.

[stellaraccident]

In D92007#2414514, @mehdi_amini wrote:

In D92007#2414495, @GeorgeLyon wrote:

The difference is that all the consumers of the C++ API are C++, so it is pretty straightforward to update when making breaking changes even if the surface is greater.

You're clearly not updating all of the Google internal users ;)

Language bindings will live at least partially in their own ecosystem and requiring folks who work on the C API to understand each individual ecosystem may be too onerous, even if the surface area is smaller.

There are a few aspects here that make me doubtful about it:

• the C API is more limited (by the virtue of using C: no overloading sets, no inheritance/overrides, no templates, etc.)
• the C API is intended to be stable at some point and see little churn.
• I does not seem hard to me to see myself changing the C API like here and adjust any in-tree language bindings in a fairly straightforward way, in most cases it'll be fairly mechanical to look at the API call site and update it. I don't quite get why this would be more complex than all of the C++ uses actually.

FWIW - I think we will eventually want these bindings updated as part of making changes to the C-API that they ride on top of. But with this as an optional feature that we have not yet finished/enabled by default, we're in an intermediate state. Once we have build bots on this and a reasonable path to build locally (still shaving some things there), I'll be sending an RFC to enable by default -- and then updating will be required.

[zhanghb97]

I'm so sorry for the late reply because of the time zone difference, and thanks for your help and review!

mlir/lib/Bindings/Python/IRModules.cpp

createCustomDialectWrapper(const std::string &dialectNamespace,
   if (!dialectClass) {
     // Use the base class.
     return py::cast(PyDialect(std::move(dialectDescriptor)));
   }
 
   // Create the custom implementation.
   return (*dialectClass)(std::move(dialectDescriptor));
 }
+
+static MlirStringRef toMlirStringRef(const std::string &s) {
+  return mlirStringRefCreate(s.data(), s.size());
+}
+
 //------------------------------------------------------------------------------
 // Collections.
 //------------------------------------------------------------------------------
 
 namespace {
 
 class PyRegionIterator {
 public:
for (auto *successor : *successors) {
       if (!successor)
         throw SetPyError(PyExc_ValueError, "successor block cannot be None");
       mlirSuccessors.push_back(successor->get());
     }
   }
 
   // Apply unpacked/validated to the operation state. Beyond this
   // point, exceptions cannot be thrown or else the state will leak.
-  MlirOperationState state = mlirOperationStateGet(name.c_str(), location->loc);
+  MlirOperationState state =
+      mlirOperationStateGet(toMlirStringRef(name), location->loc);
   if (!mlirOperands.empty())
     mlirOperationStateAddOperands(&state, mlirOperands.size(),
                                   mlirOperands.data());
   if (!mlirResults.empty())
     mlirOperationStateAddResults(&state, mlirResults.size(),
                                  mlirResults.data());
   if (!mlirAttributes.empty()) {
     // Note that the attribute names directly reference bytes in
     // mlirAttributes, so that vector must not be changed from here
     // on.
     llvm::SmallVector<MlirNamedAttribute, 4> mlirNamedAttributes;
     mlirNamedAttributes.reserve(mlirAttributes.size());
     for (auto &it : mlirAttributes)
       mlirNamedAttributes.push_back(
-          mlirNamedAttributeGet(it.first.c_str(), it.second));
+          mlirNamedAttributeGet(toMlirStringRef(it.first), it.second));
     mlirOperationStateAddAttributes(&state, mlirNamedAttributes.size(),
                                     mlirNamedAttributes.data());
   }
   if (!mlirSuccessors.empty())
     mlirOperationStateAddSuccessors(&state, mlirSuccessors.size(),
                                     mlirSuccessors.data());
   if (regions) {
     llvm::SmallVector<MlirRegion, 4> mlirRegions;

 }
 
 //------------------------------------------------------------------------------
 // PyNamedAttribute.
 //------------------------------------------------------------------------------
 
 PyNamedAttribute::PyNamedAttribute(MlirAttribute attr, std::string ownedName)
     : ownedName(new std::string(std::move(ownedName))) {
-  namedAttr = mlirNamedAttributeGet(this->ownedName->c_str(), attr);
+  namedAttr = mlirNamedAttributeGet(toMlirStringRef(*this->ownedName), attr);
 }
 
 //------------------------------------------------------------------------------
 // PyType.
 //------------------------------------------------------------------------------
 
 bool PyType::operator==(const PyType &other) {
   return mlirTypeEqual(type, other.type);

 
 /// A list of operation attributes. Can be indexed by name, producing
 /// attributes, or by index, producing named attributes.
 class PyOpAttributeMap {
 public:
   PyOpAttributeMap(PyOperationRef operation) : operation(operation) {}
 
   PyAttribute dunderGetItemNamed(const std::string &name) {
-    MlirAttribute attr =
-        mlirOperationGetAttributeByName(operation->get(), name.c_str());
+    MlirAttribute attr = mlirOperationGetAttributeByName(operation->get(),
+                                                         toMlirStringRef(name));
     if (mlirAttributeIsNull(attr)) {
       throw SetPyError(PyExc_KeyError,
                        "attempt to access a non-existent attribute");
     }
     return PyAttribute(operation->getContext(), attr);
   }
 
   PyNamedAttribute dunderGetItemIndexed(intptr_t index) {
     if (index < 0 || index >= dunderLen()) {
       throw SetPyError(PyExc_IndexError,
                        "attempt to access out of bounds attribute");
     }
     MlirNamedAttribute namedAttr =
         mlirOperationGetAttribute(operation->get(), index);
-    return PyNamedAttribute(namedAttr.attribute, std::string(namedAttr.name));
+    return PyNamedAttribute(namedAttr.attribute,
+                            std::string(namedAttr.name.data));
   }
 
   void dunderSetItem(const std::string &name, PyAttribute attr) {
-    mlirOperationSetAttributeByName(operation->get(), name.c_str(), attr.attr);
+    mlirOperationSetAttributeByName(operation->get(), toMlirStringRef(name),
+                                    attr.attr);
   }
 
   void dunderDelItem(const std::string &name) {
-    int removed =
-        mlirOperationRemoveAttributeByName(operation->get(), name.c_str());
+    int removed = mlirOperationRemoveAttributeByName(operation->get(),
+                                                     toMlirStringRef(name));
     if (!removed)
       throw SetPyError(PyExc_KeyError,
                        "attempt to delete a non-existent attribute");
   }
 
   intptr_t dunderLen() {
     return mlirOperationGetNumAttributes(operation->get());
   }
 
   bool dunderContains(const std::string &name) {
-    return !mlirAttributeIsNull(
-        mlirOperationGetAttributeByName(operation->get(), name.c_str()));
+    return !mlirAttributeIsNull(mlirOperationGetAttributeByName(
+        operation->get(), toMlirStringRef(name)));
   }
 
   static void bind(py::module &m) {
     py::class_<PyOpAttributeMap>(m, "OpAttributeMap")
         .def("__contains__", &PyOpAttributeMap::dunderContains)
         .def("__len__", &PyOpAttributeMap::dunderLen)
         .def("__getitem__", &PyOpAttributeMap::dunderGetItemNamed)
         .def("__getitem__", &PyOpAttributeMap::dunderGetItemIndexed)
py::class_<PyLocation>(m, "Location")
                               mlirLocationUnknownGet(context->get()));
           },
           py::arg("context") = py::none(),
           "Gets a Location representing an unknown location")
       .def_static(
           "file",
           [](std::string filename, int line, int col,
              DefaultingPyMlirContext context) {
-            return PyLocation(context->getRef(),
-                              mlirLocationFileLineColGet(
-                                  context->get(), filename.c_str(), line, col));
+            return PyLocation(
+                context->getRef(),
+                    context->get(), toMlirStringRef(filename), line, col));
           },
           py::arg("filename"), py::arg("line"), py::arg("col"),
           py::arg("context") = py::none(), kContextGetFileLocationDocstring)
       .def_property_readonly(
           "context",
           [](PyLocation &self) { return self.getContext().getObject(); },
           "Context that owns the Location")
       .def("__repr__", [](PyLocation &self) {
         PyPrintAccumulator printAccum;
         mlirLocationPrint(self.loc, printAccum.getCallback(),
                           printAccum.getUserData());
         return printAccum.join();
       });
 
   //----------------------------------------------------------------------------
   // Mapping of Module
   //----------------------------------------------------------------------------
   py::class_<PyModule>(m, "Module")
       .def_property_readonly(MLIR_PYTHON_CAPI_PTR_ATTR, &PyModule::getCapsule)
       .def(MLIR_PYTHON_CAPI_FACTORY_ATTR, &PyModule::createFromCapsule)
       .def_static(
           "parse",
           [](const std::string moduleAsm, DefaultingPyMlirContext context) {
-            MlirModule module =
-                mlirModuleCreateParse(context->get(), moduleAsm.c_str());
+            MlirModule module = mlirModuleCreateParse(
+                context->get(), toMlirStringRef(moduleAsm));
             // TODO: Rework error reporting once diagnostic engine is exposed
             // in C API.
             if (mlirModuleIsNull(module)) {
               throw SetPyError(
                   PyExc_ValueError,
                   "Unable to parse module assembly (see diagnostics)");
             }
             return PyModule::forModule(module).releaseObject();
void mlir::python::populateIRSubmodule(py::module &m) {
 
   //----------------------------------------------------------------------------
   // Mapping of PyAttribute.
   //----------------------------------------------------------------------------
   py::class_<PyAttribute>(m, "Attribute")
       .def_static(
           "parse",
           [](std::string attrSpec, DefaultingPyMlirContext context) {
-            MlirAttribute type =
-                mlirAttributeParseGet(context->get(), attrSpec.c_str());
+            MlirAttribute type = mlirAttributeParseGet(
+                context->get(), toMlirStringRef(attrSpec));
             // TODO: Rework error reporting once diagnostic engine is exposed
             // in C API.
             if (mlirAttributeIsNull(type)) {
               throw SetPyError(PyExc_ValueError,
                                llvm::Twine("Unable to parse attribute: '") +
                                    attrSpec + "'");
             }
             return PyAttribute(context->getRef(), type);
void mlir::python::populateIRSubmodule(py::module &m) {
   //----------------------------------------------------------------------------
   // Mapping of PyNamedAttribute
   //----------------------------------------------------------------------------
   py::class_<PyNamedAttribute>(m, "NamedAttribute")
       .def("__repr__",
            [](PyNamedAttribute &self) {
              PyPrintAccumulator printAccum;
              printAccum.parts.append("NamedAttribute(");
-             printAccum.parts.append(self.namedAttr.name);
+             printAccum.parts.append(self.namedAttr.name.data);
              printAccum.parts.append("=");
              mlirAttributePrint(self.namedAttr.attribute,
                                 printAccum.getCallback(),
                                 printAccum.getUserData());
              printAccum.parts.append(")");
              return printAccum.join();
            })
       .def_property_readonly(
           "name",
           [](PyNamedAttribute &self) {
-            return py::str(self.namedAttr.name, strlen(self.namedAttr.name));
+            return py::str(self.namedAttr.name.data,
+                           self.namedAttr.name.length);
           },
           "The name of the NamedAttribute binding")
       .def_property_readonly(
           "attr",
           [](PyNamedAttribute &self) {
             // TODO: When named attribute is removed/refactored, also remove
             // this constructor (it does an inefficient table lookup).
             auto contextRef = PyMlirContext::forContext(
void mlir::python::populateIRSubmodule(py::module &m) {
 
   //----------------------------------------------------------------------------
   // Mapping of PyType.
   //----------------------------------------------------------------------------
   py::class_<PyType>(m, "Type")
       .def_static(
           "parse",
           [](std::string typeSpec, DefaultingPyMlirContext context) {
-            MlirType type = mlirTypeParseGet(context->get(), typeSpec.c_str());
+            MlirType type =
+                mlirTypeParseGet(context->get(), toMlirStringRef(typeSpec));
             // TODO: Rework error reporting once diagnostic engine is exposed
             // in C API.
             if (mlirTypeIsNull(type)) {
               throw SetPyError(PyExc_ValueError,
                                llvm::Twine("Unable to parse type: '") +
                                    typeSpec + "'");
             }
             return PyType(context->getRef(), type);

mlir/lib/Bindings/Python/PybindUtils.h

 
 #include "mlir-c/Support.h"
 #include "llvm/ADT/Optional.h"
 #include "llvm/ADT/Twine.h"
 
 #include <pybind11/pybind11.h>
 #include <pybind11/stl.h>
 
-
 namespace mlir {
 namespace python {
 
 // Sets a python error, ready to be thrown to return control back to the
 // python runtime.
 // Correct usage:
 //   throw SetPyError(PyExc_ValueError, "Foobar'd");
 pybind11::error_already_set SetPyError(PyObject *excClass,

 /// Accumulates into a python string from a method that accepts an
 /// MlirStringCallback.
 struct PyPrintAccumulator {
   pybind11::list parts;
 
   void *getUserData() { return this; }
 
   MlirStringCallback getCallback() {
-    return [](const char *part, intptr_t size, void *userData) {
+    return [](MlirStringRef part, void *userData) {
       PyPrintAccumulator *printAccum =
           static_cast<PyPrintAccumulator *>(userData);
-      pybind11::str pyPart(part, size); // Decodes as UTF-8 by default.
+      pybind11::str pyPart(part.data,
+                           part.length); // Decodes as UTF-8 by default.
       printAccum->parts.append(std::move(pyPart));
     };
   }
 
   pybind11::str join() {
     pybind11::str delim("", 0);
     return delim.attr("join")(parts);
   }
 };
 
 /// Accumulates int a python file-like object, either writing text (default)
 /// or binary.
 class PyFileAccumulator {
 public:
   PyFileAccumulator(pybind11::object fileObject, bool binary)
       : pyWriteFunction(fileObject.attr("write")), binary(binary) {}
 
   void *getUserData() { return this; }
 
   MlirStringCallback getCallback() {
-    return [](const char *part, intptr_t size, void *userData) {
+    return [](MlirStringRef part, void *userData) {
       pybind11::gil_scoped_acquire();
       PyFileAccumulator *accum = static_cast<PyFileAccumulator *>(userData);
       if (accum->binary) {
         // Note: Still has to copy and not avoidable with this API.
-        pybind11::bytes pyBytes(part, size);
+        pybind11::bytes pyBytes(part.data, part.length);
         accum->pyWriteFunction(pyBytes);
       } else {
-        pybind11::str pyStr(part, size); // Decodes as UTF-8 by default.
+        pybind11::str pyStr(part.data,
+                            part.length); // Decodes as UTF-8 by default.
         accum->pyWriteFunction(pyStr);
       }
     };
   }
 
 private:
   pybind11::object pyWriteFunction;
   bool binary;
 };
 
 /// Accumulates into a python string from a method that is expected to make
 /// one (no more, no less) call to the callback (asserts internally on
 /// violation).
 struct PySinglePartStringAccumulator {
   void *getUserData() { return this; }
 
   MlirStringCallback getCallback() {
-    return [](const char *part, intptr_t size, void *userData) {
+    return [](MlirStringRef part, void *userData) {
       PySinglePartStringAccumulator *accum =
           static_cast<PySinglePartStringAccumulator *>(userData);
       assert(!accum->invoked &&
              "PySinglePartStringAccumulator called back multiple times");
       accum->invoked = true;
-      accum->value = pybind11::str(part, size);
+      accum->value = pybind11::str(part.data, part.length);
     };
   }
 
   pybind11::str takeValue() {
     assert(invoked && "PySinglePartStringAccumulator not called back");
     return std::move(value);
   }