Skip to content

dialects: (hw) Modernise dialect with properties instead of attributes#5320

Draft
gabrielrodcanal wants to merge 4 commits intomainfrom
gabriel/modernise_hw_dialect
Draft

dialects: (hw) Modernise dialect with properties instead of attributes#5320
gabrielrodcanal wants to merge 4 commits intomainfrom
gabriel/modernise_hw_dialect

Conversation

@gabrielrodcanal
Copy link
Contributor

Attributes are replaced by properties to be compliant with latest MLIR versions (tested with 22.0.0).

@gabrielrodcanal gabrielrodcanal self-assigned this Oct 6, 2025
@gabrielrodcanal gabrielrodcanal added the dialects Changes on the dialects label Oct 6, 2025
@gabrielrodcanal gabrielrodcanal marked this pull request as draft October 6, 2025 16:50
@gabrielrodcanal
Copy link
Contributor Author

The method parse_optional_attr_dict_with_reserved_attr_names was used to parse attributes provided their names. There is currently not an equivalent method for properties @alexarice @superlopuh

@superlopuh
Copy link
Member

Two thoughts:

  1. We want to stick to being compatible with the same mlir-opt version in all the dialects, these changes seem to actually be to one of the dialects not in upstream MLIR, and in CIRCT, so it's probably OK for now, but we'll need to add some kind of mechanism to check compatibility, probably by moving this to a different repo
  2. My understanding is that the attributes vs properties logic is a bit more interesting, in that with the right flag properties are actually printed in the attribute dict. We handle this correctly in custom assembly format, but not in the exposed printer APIs, could you please add a new function to Printer and Parser with the right logic? I expect it to be a fairly straightforward move of logic from the declarative assembly format

@gabrielrodcanal
Copy link
Contributor Author

Two thoughts:

  1. We want to stick to being compatible with the same mlir-opt version in all the dialects, these changes seem to actually be to one of the dialects not in upstream MLIR, and in CIRCT, so it's probably OK for now, but we'll need to add some kind of mechanism to check compatibility, probably by moving this to a different repo
  2. My understanding is that the attributes vs properties logic is a bit more interesting, in that with the right flag properties are actually printed in the attribute dict. We handle this correctly in custom assembly format, but not in the exposed printer APIs, could you please add a new function to Printer and Parser with the right logic? I expect it to be a fairly straightforward move of logic from the declarative assembly format

@gabrielrodcanal
Copy link
Contributor Author

gabrielrodcanal commented Oct 6, 2025

Two thoughts:

  1. We want to stick to being compatible with the same mlir-opt version in all the dialects, these changes seem to actually be to one of the dialects not in upstream MLIR, and in CIRCT, so it's probably OK for now, but we'll need to add some kind of mechanism to check compatibility, probably by moving this to a different repo
  2. My understanding is that the attributes vs properties logic is a bit more interesting, in that with the right flag properties are actually printed in the attribute dict. We handle this correctly in custom assembly format, but not in the exposed printer APIs, could you please add a new function to Printer and Parser with the right logic? I expect it to be a fairly straightforward move of logic from the declarative assembly format

Respect to the second thought, it turns out it's not necessary to change anything in the dialect. Just by keeping parse_optional_attr_dict_with_reserved_attr_names roundtripping works fine.

However, I've seen that some tests try to add custom attributes, such as here

hw.instance "empty_instance_with_attrs" @test_empty() -> () {foo = 4 : i32, bar}
How is this usually handled in xDSL when properties are expected? Is compatibility for attributes added too? Or maybe we don't really need support for this and the test can be updated accordingly?

@superlopuh
Copy link
Member

I'm a bit confused by your question, what doesn't work? This is one of those things that the custom syntax must support, all ops can have extra attributes added that are not in the definition. I have a feeling that a bunch of our custom printers/parsers don't handle it properly but we should.

@gabrielrodcanal
Copy link
Contributor Author

I think there might be a little misunderstanding on the support of attributes and properties on my end. As far as I know MLIR supports both attributes and properties at the same time, correct? Does that mean that for a case like

hw.instance "empty_instance_with_attrs" @test_empty() -> () {foo = 4 : i32, bar}

properties and attributes need to be parsed separately? As far as I know it is possible to have custom attributes but not custom properties. Sorry if it's basic, but I've never had to mix attributes and properties before.

@superlopuh
Copy link
Member

It's a little weird, but since properties were added later and as a kind of optimisation over attributes, there are some places where they behave in a slightly weird way. One case of this is memref.load, where because it's annotated with ParsePropInAttrDict, the nontemporal property is printed in the attr dict in the custom format.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

dialects Changes on the dialects

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants