search

xdslproject / xdsl

A Python Compiler Design Toolkit
Other
278 stars 72 forks source link

core: Properties vs Attributes #3260

Open alexarice opened 1 month ago

alexarice commented 1 month ago

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 using prop_def.

Would it be worth spending a morning/day to update all of these?

superlopuh commented 1 month ago

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

alexarice commented 1 month ago

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.

math-fehr commented 1 month ago

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.

alexarice commented 1 month ago

What do you mean by "conversion between properties and attributes by default"?

math-fehr commented 1 month ago

If you parse an operation with an attribute "a" when a property "a" is expected, the attribute is transformed into a property.

alexarice commented 1 month ago

In mlir or xdsl

math-fehr commented 1 month ago

Both

alexarice commented 1 month ago

Ah ok, maybe this is less of an issue than I thought then, though the difference between the two still annoys me

math-fehr commented 1 month ago

Yeah I agree, we should try to update MLIR soon, so we can resolve this issue. I won't really have time until PLDI (mid november), but I could take a look after for updating it.

alexarice commented 1 month ago

Would be good to make a list of the subtasks that need to be done. The main one I'm aware of is updating scf.parallel/scf.reduce. Do you know which commit you want to aim for?

math-fehr commented 1 month ago

I would just take one of the latest commits that pass, and see what breaks in our tests. Besides that, I don't think there is a fundamental change in MLIR core that we have to handle, so I think it should just be a matter of fixing some dialect tests.