[mlir] Add property combinators, initial ODS support #94732
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
While we have had a Properties.td that allowed for defining non-attribute-backed properties, such properties were not plumbed through the basic autogeneration facilities available to attributes, forcing those who want to migrate to the new system to write such code by hand. ## Potentially breaking changes - The `setFoo()` methods on `Properties` struct no longer take their inputs by const reference. Those wishing to pass non-owned values of a property by reference to constructors and setters should set the interface type to `const [storageType]&` - Adapters and operations now define getters and setters for properties listed in ODS, which may conflict with custom getters. - Builders now include properties listed in ODS specifications, potentially conflicting with custom builders with the same type signature. ## Extensions to the `Property` class This pull request adds several fields to the `Property` class, including: - `parser`, `optionalParser`, and `printer` (for parsing/printing properties of a given type in ODS syntax) - `storageTypeValueOverride`, an extension of `defaultValue` to allow the storage and interface type defaults to differ - `baseProperty` (allowing for classes like `DefaultValuedProperty`) Existing fields have also had their documentation comments updated. This commit does not add a `PropertyConstraint` analogous to `AttrConstraint`, but this is a natural evolution of the work here. This commit also adds the concrete property kinds `I32Property`, `I64Property`, `UnitProperty` (and special handling for it like for UnitAttr), and `BoolProperty`. ## Property combinators `Properties.td` also now includes several ways to combine properties. One is `ArrayProperty<Property elem>`, which now stores a variable-length array of some property as `SmallVector<elem.storageType>` and uses `ArrayRef<elem.storageType>` as its interface type. It has `IntArrayProperty` subclasses that change its conversion to attributes to use `DenseI[N]Attr`s instead of an `ArrayAttr`. Similarly, `OptionalProperty<Property p>` wraps a property's storage in `std::optional<>` and adds a `std::nullopt` default value. In the case where the underlying property can be parsed optionally but doesn't have its own default value, `OptionalProperty` can piggyback off the optional parser to produce a cleaner syntax, as opposed to its general form, which is either `none` or `some<[value]>`. (Note that `OptionalProperty` can be nested if desired). ## Autogeneration changes Operations and adaptors now support getters and setters for properties like those for attributes. Unlike for attributes, there aren't separate value and attribute forms, since there is no `FooAttr()` available for a `getFooAttr()` to return. The largest change is to operation formats. Previously, properties could only be used in custom directives. Now, they can be used anywhere an attribute could be used, and have parsers and printers defined in their tablegen records. These updates include special `UnitProperty` logic like that used for `UnitAttr`. ## Misc. Some attempt has been made to test the new functionality. This commit takes tentative steps towards updating the documentation to account for properties. A full update will be in order once any followup work has been completed and the interfaces have stabilized.
|
@llvm/pr-subscribers-mlir-core @llvm/pr-subscribers-mlir Author: Krzysztof Drewniak (krzysz00) ChangesWhile we have had a Properties.td that allowed for defining non-attribute-backed properties, such properties were not plumbed through the basic autogeneration facilities available to attributes, forcing those who want to migrate to the new system to write such code by hand. Potentially breaking changes
Extensions to the
|
|
Ping |
|
Thanks for this contribution! This is awesome to see progress on properties, it's part of my TODO list but I'm swamped, so many thanks here. @Mogball may have cycles to look at this, I started looking but think I won't get to it in details before a week. |
|
Yeah, I started off wanting to plumb My long-term hope is that we'll be able to replace many uses of inherent attributes with inherent properties and save ourselves a bunch of hash lookups for basic data |
joker-eph
reviewed
Jun 13, 2024
| /// `SmallVector` of that property. This uses an `ArrayAttr` as its attribute form | ||
| /// though subclasses can override this, as is the case with IntArrayAttr below. | ||
| class ArrayProperty<Property elem = Property<>, string desc = ""> : | ||
| Property<"::llvm::SmallVector<" # elem.storageType # ">", desc> { |
There was a problem hiding this comment.
If we use a SmallVector we likely should expose to the user the size to use for the SmallVector I think.
There was a problem hiding this comment.
Might be a good idea to somehow make that an optional argument to the tablegen
| @@ -1026,6 +1055,8 @@ to: | |||
| 1. All operand and result types must appear within the format using the various | |||
| `type` directives, either individually or with the `operands` or `results` | |||
| directives. | |||
| 1. Unless all non-attribute properties appear in the format, the `prop-dict` | |||
| directive must be present. | |||
There was a problem hiding this comment.
I'm actually working on a patch to go further in the proper separation of discardable and inherent attribute to force prop-dict to be present when some inherent attributes aren't present in the format (just a comment passing by, nothing blocking your work of course)
Co-authored-by: Mehdi Amini <[email protected]>
Dinistro
reviewed
Jun 18, 2024
Co-authored-by: Christian Ulmann <[email protected]>
Co-authored-by: Christian Ulmann <[email protected]>
|
Does someone have time to do another review round of this? I'll try to allocate time for another round, but I do not feel qualified to accept this in the end. |
Mogball
approved these changes
Jul 23, 2024
There was a problem hiding this comment.
I mean this largely looks fine to me. I didn't do a detailed review, but I agree with the overall direction here. I also experienced a QoL downgrade when moving ops to properties because there wasn't the same level of ODS support for properties. I previously tried to add basic getters and setters for properties, but @joker-eph had concerns that it would encourage poor performance patterns.
On the other hand, since I recently made a bunch of these things generate inline in ODS, I checked the generated assembly at the time to ensure that there weren't actually any costs to doing
int a = op.getPropertyFoo();
int b = op.getPropertyBar();Versus
auto &props = op.getProperties();
int a = props.foo;
int b = props.bar;Assuming the return type is trivial.
General feedback here is that this seems like it could have been split up between multiple PRs: the ODS changes could have been made separately to adding all the new property combinators.
|
Agreed that this could probably be multiple PRs - this is a change that grew, honestly. I can go looking for split points. @Mogball , on the topic of the change growing, I was thinking that support for something like |
|
It's fine for now. Just for future reference :) |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While we have had a Properties.td that allowed for defining non-attribute-backed properties, such properties were not plumbed through the basic autogeneration facilities available to attributes, forcing those who want to migrate to the new system to write such code by hand.
Potentially breaking changes
setFoo()methods onPropertiesstruct no longer take their inputs by const reference. Those wishing to pass non-owned values of a property by reference to constructors and setters should set the interface type toconst [storageType]&Extensions to the
PropertyclassThis commit adds several fields to the
Propertyclass, including:parser,optionalParser, andprinter(for parsing/printing properties of a given type in ODS syntax)storageTypeValueOverride, an extension ofdefaultValueto allow the storage and interface type defaults to differbaseProperty(allowing for classes likeDefaultValuedProperty)Existing fields have also had their documentation comments updated.
This commit does not add a
PropertyConstraintanalogous toAttrConstraint, but this is a natural evolution of the work here.This commit also adds the concrete property kinds
I32Property,I64Property,UnitProperty(and special handling for it like for UnitAttr), andBoolProperty.Property combinators
Properties.tdalso now includes several ways to combine properties.One is
ArrayProperty<Property elem>, which now stores a variable-length array of some property asSmallVector<elem.storageType>and usesArrayRef<elem.storageType>as its interface type. It hasIntArrayPropertysubclasses that change its conversion to attributes to useDenseI[N]Attrs instead of anArrayAttr.Similarly,
OptionalProperty<Property p>wraps a property's storage instd::optional<>and adds astd::nulloptdefault value. In the case where the underlying property can be parsed optionally but doesn't have its own default value,OptionalPropertycan piggyback off the optional parser to produce a cleaner syntax, as opposed to its general form, which is eithernoneorsome<[value]>.(Note that
OptionalPropertycan be nested if desired).Autogeneration changes
Operations and adaptors now support getters and setters for properties like those for attributes. Unlike for attributes, there aren't separate value and attribute forms, since there is no
FooAttr()available for agetFooAttr()to return.The largest change is to operation formats. Previously, properties could only be used in custom directives. Now, they can be used anywhere an attribute could be used, and have parsers and printers defined in their tablegen records.
These updates include special
UnitPropertylogic like that used forUnitAttr.Misc.
Some attempt has been made to test the new functionality.
This commit takes tentative steps towards updating the documentation to account for properties. A full update will be in order once any followup work has been completed and the interfaces have stabilized.