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);
?