-
Notifications
You must be signed in to change notification settings - Fork 72
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
core: Properties vs Attributes #3260
Comments
Could you please rephrase this issue in terms of an actionable thing to do? I'm not sure how urgent this is since both MLIR and xDSL are quite lenient when it comes to people putting things in the wrong place, like attributes in properties and vice versa |
The actionable thing to do would be to transition from using an attribute to a property when MLIR uses a property. Personally, I feel that this is worthwhile in order to keep the generic syntax the same between both libraries. |
I think this needs to happen at the same time that we are doing an MLIR update. Updating it without updating MLIR is a bit weird, even though there is a conversion between properties and attributes by default. |
What do you mean by "conversion between properties and attributes by default"? |
If you parse an operation with an attribute "a" when a property "a" is expected, the attribute is transformed into a property. |
In mlir or xdsl |
Both |
Ah ok, maybe this is less of an issue than I thought then, though the difference between the two still annoys me |
Yeah I agree, we should try to update MLIR soon, so we can resolve this issue. |
Would be good to make a list of the subtasks that need to be done. The main one I'm aware of is updating |
I would just take one of the latest commits that pass, and see what breaks in our tests. |
At some point mlir switched over from using attributes to properties, but still calls everything an Attribute in tablegen. As a result, there are a lot of places where we are using
attr_def
where we should likely be usingprop_def
.Would it be worth spending a morning/day to update all of these?
The text was updated successfully, but these errors were encountered: