-
Notifications
You must be signed in to change notification settings - Fork 77
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: (irdl) Add regions to declarative assembly format #3065
core: (irdl) Add regions to declarative assembly format #3065
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you comment on why regions must come after attributes? I can't see anything about this in the mlir documentation (which doesn't mean it's not true)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Haven't read through everything but here's some comments
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
…/github.com/xdslproject/xdsl into kim/declarative-assembly-parser/add-regions
…s' into kim/declarative-assembly-parser/add-regions. Remove casts.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's still two files that you've not touched being pulled in somehow. I've left a few minor comments but looking good, will give it a test tomorrow
I also have no idea why the mlir integration tests are failing |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3065 +/- ##
==========================================
+ Coverage 89.88% 89.90% +0.02%
==========================================
Files 418 418
Lines 52764 52913 +149
Branches 8175 8202 +27
==========================================
+ Hits 47426 47571 +145
- Misses 4013 4015 +2
- Partials 1325 1327 +2 ☔ View full report in Codecov by Sentry. |
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
Co-authored-by: Sasha Lopoukhine <superlopuh@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Amazing to see that in!
The implementation and existing tests seem perfect to me, only have some minor polish comments :) Making this an approval as at this point I consider any further polishing a bonus, the feature seems ready to benefit xDSL in itself!
Thanks a lot!
Fix region directive formats to be consistent by changing them to dollar-ident
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've had a little test locally and this seems reasonable. Thanks for implementing this. Happy for this to be merged once the remaining comments are resolved
Added support for regions to the declarative assembly format.
The declarative assembly format now supports:
The format requires that:
Also added tests.