Details
Diff Detail
- Repository
 - rG LLVM Github Monorepo
 
Event Timeline
| mlir/lib/Bindings/Python/IRCore.cpp | ||
|---|---|---|
| 3311 | In other places I see "cheap" functions like this are usually exposed as properties. There's also the class-level variant def_property_readonly_static.  | |
| mlir/lib/Bindings/Python/IRModule.h | ||
| 893 | Can you add this to mlir_type_subclass as well, and implement it for some non-builtin types as an example.  | |
| 896 | Instead of returning None, could we just skip defining the function? That would make it obvious when a class doesn't have it implemented.  | |
| mlir/test/python/ir/builtin_types.py | ||
| 569–571 | maybe just define these as a single list of tuples?  | |
| 624 | I would expect get_typeid() here to work the same as Type.get_typeid(), and return the concrete typeid (the id for vector in this case).  | |
incorporate comments
| mlir/lib/Bindings/Python/IRModule.h | ||
|---|---|---|
| 896 | I don't understand why (some pybind impl detail) but skipping defining on one such concrete leaves a property object descriptor there; i.e., if (DerivedTy::getTypeIdFunction)
  cls.def_property_readonly_static(
      "typeid", [](py::object & /*class*/) -> MlirTypeID {
        return DerivedTy::getTypeIdFunction();
      });results in <property object at 0x7efe2dcebc40> being printed for ShapeType.typeid. Could do if (DerivedTy::getTypeIdFunction)
  cls.def_property_readonly_static(
      "typeid", [](py::object & /*class*/) -> MlirTypeID {
        return DerivedTy::getTypeIdFunction();
      });
else
  cls.attr("typeid") = py::none();but that just looks the same from the user side and is baroque (imho). Changed to throw a ValueError.  | |
| mlir/include/mlir-c/BuiltinTypes.h | ||
|---|---|---|
| 90 | OOC why not just MlirTypeID mlirTypeGetTypeID(MlirType type); ?  | |
| mlir/include/mlir-c/BuiltinTypes.h | ||
|---|---|---|
| 90 | You mean in general why did I add all of these () -> TypeID functions? Because in the TypeCaster diff you need to eg build a map from TypeID -> TypeCaster without having a type on hand.  | |
| mlir/include/mlir-c/BuiltinTypes.h | ||
|---|---|---|
| 90 | if only we could generate all the C api boilerplate :)  | |
| mlir/lib/Bindings/Python/IRCore.cpp | ||
|---|---|---|
| 3317–3318 | Let's use C++ API: throw py::value_error(...). Here and below.  | |
| mlir/lib/Bindings/Python/IRCore.cpp | ||
|---|---|---|
| 3317–3318 | Just clarifying where "below" refers to: there are ~50 instances of throw SetPyError in this directory - do you want me to replace all of them? If so, no problem, but that would be better in a separate diff? Otherwise ctrl+f in this UI, I see this one and the one in PyConcreteType::castFrom.  | |
| mlir/lib/Bindings/Python/IRCore.cpp | ||
|---|---|---|
| 3317–3318 | I meant only the ones introduced in this diff. Though changing all the others in a separate diff is also much appreciated.  | |
OOC why not just
MlirTypeID mlirTypeGetTypeID(MlirType type);
?