Changing prefixing for optional UnitAttr accessors to follow LLVM convention.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Nice, overall looks good and reads nicer.
mlir/include/mlir/TableGen/Operator.h | ||
---|---|---|
348 | Why can't this just be getGetterName and these merged? Computational efficiency? | |
mlir/lib/Dialect/Linalg/Transforms/ConvertToDestinationStyle.cpp | ||
304 | Should this just be isNofold() ? (not sure why attr post fix, not specific to your change though so just OOC) | |
mlir/tools/mlir-tblgen/OpDefinitionsGen.cpp | ||
1022 | Lets end with s/.'/; method will be removed 2023-06-23/ and could send a PR to https://github.com/llvm/mlir-www/blob/main/website/content/deprecation/_index.md to add it there too. | |
1382 | Could you tag all of these with same date so it is easier to find ? |
One thing that's kind of popped up while reading the review is that this going to skew towards attributes with certain naming schemes, which needs to be documented. This is a bit because I feel that 'is' doesn't read well for a number of different cases.
mlir/lib/Conversion/NVGPUToNVVM/NVGPUToNVVM.cpp | ||
---|---|---|
291 | This does not seem desirable to me that the accessor for getting the attribute name is prefixed with is instead of get. |
mlir/include/mlir/TableGen/Operator.h | ||
---|---|---|
348 | Yes, the unit attribute check is unnecessary for operands, results and other types. Open to suggestions to improve this logic though. | |
mlir/lib/Dialect/Linalg/Transforms/ConvertToDestinationStyle.cpp | ||
304 | Probably. Not sure where this specific instance of the nofold attribute accessor is defined/generated though. |
Is this just a matter of updating the documentation/style guide or is there a bigger problem regarding naming of unit attributes currently and in the future?
mlir/lib/Conversion/NVGPUToNVVM/NVGPUToNVVM.cpp | ||
---|---|---|
291 | Reverting the changes for the attribute name getters |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
---|---|---|
828 | related to what others said, this reads weird now hasInserts() would be nice if want to start with a verb |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
---|---|---|
828 | There are other cases in addition to attribute names that start with "has" where the additional "is" prefix makes it read weird. Handling each case individually does not guarantee consistency and probably isn't feasible. It may be better to update the style guide, so the programmer can ensure proper naming conventions. |
mlir/lib/Dialect/SparseTensor/Transforms/SparseTensorCodegen.cpp | ||
---|---|---|
828 | is is a verb. But this goes to River's point below: the prefix works better if one follows naming convention accounting for it. For boolean accessors is and has are common conventions for prefixing boolean return functions. @lattner commented in the linked issue that in LLVM is is the prefix convention followed. So 4 options here:
For (4) an entry is added to ODS style guide and folks update so that it reads more naturally. |
Would it be possible/reasonable to default to "is" and then allow another ODS property to overload it with an arbitrary word like "has"?
It would yes, but there was opposition to enabling accessors to be customized previously (https://github.com/llvm/llvm-project/issues/51258). This is option (3) above though.
It is unclear how much option (3) benefits vs (2) e.g., with (2) you'd have "has_padding" or "is_continuous" in the printed IR rather than "padding" and "continuous", but no need to do anything in ODS or introduce new class for UnitAttr instances to instantiate. And I almost feel that is more readable. We could also do variant of (2) where it is required that there be a prefix in the Attribute ([a-z]*_ or perhaps just an allowed set), else it gets an is.
Why can't this just be getGetterName and these merged? Computational efficiency?