- Generic mlir.ir.Attribute class.
- First standard attribute (mlir.ir.StringAttr), following the same pattern as generic vs standard types.
- NamedAttribute class.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Nice
mlir/include/mlir-c/IR.h | ||
---|---|---|
339 | S/type/attribute/? | |
mlir/lib/Bindings/Python/IRModules.cpp | ||
159 | Throwing vs returning a null? We've mostly returned nulls elsewhere (I have not followed this part of discussion, so perhaps this has been discussed already). | |
167 | What about classTy? Or, as only 3 lines, c. | |
340 | Up to you, but formatv may be more concise | |
mlir/lib/Bindings/Python/IRModules.h | ||
76 | Mmm, I thought the name would be in the MLIR context rather than owned here. |
mlir/lib/Bindings/Python/IRModules.cpp | ||
---|---|---|
80 | Can you add a class doc? | |
159 | Seems like we associate a semantic of cast (that would assert in C++ land) instead of dyn_cast to these conversion? | |
176 | Can you add a class doc? | |
mlir/lib/Bindings/Python/IRModules.h | ||
76 | This is a fallout from the C definition of MlirNamedAttribute: struct MlirNamedAttribute { const char *name; MlirAttribute attribute; }; Any chance to make the name here an Identifier instead? That would internalize the string in the context and avoid to have to use this extra management of the unique_ptr<string> done here everywhere where a named attribute will be needed. |
mlir/lib/Bindings/Python/IRModules.cpp | ||
---|---|---|
159 | Check for None and generally having type polymorphism is arguably ugly in Python. Also requires all type annotations to be Optional[Attribute] instead of just Attribute. | |
192 | Design question: do we want to have "overloads" through type dispatch or do we prefer different function names? (I personally vote for the latter) | |
338 | Let's have a todo to rework these after we expose diagnostic engine in C API | |
385 | get_named for consistency with static constructors above? I see this is not static, but it's still a constructor | |
mlir/lib/Bindings/Python/IRModules.h | ||
76 | I didn't have time to add Identifier to the C API, but it should be straightforward to do. It's the same model as attributes. |
Thanks for the reviews. I've added a number of TODOs and it doesn't look like there is disagreement on the design points. Feel free to bring them up for further discussion if you feel otherwise.
mlir/lib/Bindings/Python/IRModules.cpp | ||
---|---|---|
159 | I'm pretty sure we want to throw on a failure to "cast" via constructor invocation like this: str_attr = StringAttr(ctx.parse_attr("1")) (note that this castFrom method is not part of the Python API: it only facilitates a downcast within a constructor initializer list) Relatedly, it is more pythonic for the "default" way of using the API to raise on violation. Many APIs also have an alternative that returns None on failure/not found/etc (ex. dict["foo"] vs dict.get("foo")). I had considered adding explicit cast/dyn_cast/isa methods to follow LLVM conventions, and I think those could be valuable additions, but I left them out of this revision (there are also some fiddly things with the type hierarchy that I need to experiment with). For example, it would be nice to also have methods on Attribute like this: attr.isa(StringAttr) attr.cast(StringAttr) attr.dyn_cast(StringAttr) Whether or not we do those, I like the "constructor casts" style as a pretty good default that feels reasonable to use, even if it is effectively a short-hand for a future attr.cast(...) call (i.e. it is good form to have a constructor, which then begs the question of what it should do, and this seems reasonable). | |
167 | Changed to cls (here and in PyType). | |
192 | I tried both and prefer the latter as well. The case becomes stronger when you consider those like PyIntegerType below and I would summarize as "if it is constructing a different kind of attribute or type, it should have a different named static factory function (along with documentation)." This leaves the door open to provide default arguments as sugar, but I prefer for methods that *do* different things to actually be differently named, not rely on magic and hard to describe argument dispatch. | |
338 | Added a TODO to all of the parse_* sites. | |
340 | Thanks, leaving as is since SetPyError takes a Twine. | |
mlir/lib/Bindings/Python/IRModules.h | ||
76 | Agreed - this was quite awkward/bug prone and it would be best if the C-API did do it this way. I added a TODO to rework this to be based on Identifier. Regarding the related point, for something as basic as a (char*, size), I think it makes sense to have a local struct/calling convention for that. Keep it narrow/local and leave it to the interfacing language to do anything fancy it wants with strings. I'll attempt to resolve the TODO in a followup for both this and the C-API. |
S/type/attribute/?