Skip to content
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

Move operations to use MLIR Properties instead of Attributes #5273

Open
dtzSiFive opened this issue May 26, 2023 · 6 comments
Open

Move operations to use MLIR Properties instead of Attributes #5273

dtzSiFive opened this issue May 26, 2023 · 6 comments

Comments

@dtzSiFive
Copy link
Contributor

This would be advantageous for many operations, and in many cases should be a simple conversion.

See:

https://mlir.llvm.org/OpenMeetings/2023-02-09-Properties.pdf

https://discourse.llvm.org/t/rfc-introducing-mlir-operation-properties/67846

And recent upstream MLIR commits migrating their dialects/IR over.

@teqdruid
Copy link
Contributor

for reference, https://reviews.llvm.org/D148299 moves the linalg dialect over using the dialect flag.

@dtzSiFive
Copy link
Contributor Author

Oh, glad you're interested/following this! Thanks for the pointer, neat how they managed backwards compat!

I have a branch I've been sitting on that ports the HW-y dialects over (IIRC mostly just setting the flag, like upstream MLIR dialects did), but SV and FIRRTL will need some more careful handling/auditing before feel good about it.

And the benefit really starts once we move over and can start taking advantage of them-- just opting in in my testing only increased memory usage slightly (which makes sense, as I understand it, although didn't deep dive).

Happy to make a PR with that as a starting point if folks besides me are interested in this direction!

@teqdruid
Copy link
Contributor

I'm a big fan of properties in theory, and am very interested in moving things over. Less DictAttr building, more efficient get/set, though at the cost of more memory use in some scenarios.

If you're willing to push on it, I'd suggest moving one dialect at a time. I'll move the esi and msft dialects over when I get the chance.

@dtzSiFive
Copy link
Contributor Author

dtzSiFive commented Aug 30, 2023

This is now the default, but we can opt-out until LLVM 19 by setting let usePropertiesForAttributes = 0;. See: https://reviews.llvm.org/D158581 .

Nightly track-LLVM has started failing for (at least) this reason (edit, for clarity which "Nightly" I meant).

@teqdruid
Copy link
Contributor

Nightly has started failing for (at least) this reason

I'm not seeing the failures. Can you point me to one?

@teqdruid
Copy link
Contributor

Oh... you mean the nightly track changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants