-
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
dialects: (builtin) Parse @ module names #3664
dialects: (builtin) Parse @ module names #3664
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3664 +/- ##
==========================================
- Coverage 91.29% 91.29% -0.01%
==========================================
Files 466 466
Lines 58354 58357 +3
Branches 5639 5624 -15
==========================================
+ Hits 53276 53278 +2
Misses 3629 3629
- Partials 1449 1450 +1 ☔ View full report in Codecov by Sentry. |
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.
This looks great, although this is a good opportunity to make the module name a property, and it'll simplify the format logic:
❯ echo "module @hello {}" | mlir-opt --mlir-print-op-generic
"builtin.module"() <{sym_name = "hello"}> ({
^bb0:
}) : () -> ()
Does this work?
|
You mean does the combination of module name and attribute work? It does, added a test for it. |
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.
Looks good, I would just change a bit the init with my comment
xdsl/dialects/builtin.py
Outdated
@@ -1643,21 +1643,30 @@ def __init__( | |||
self, | |||
ops: list[Operation] | Region, | |||
attributes: Mapping[str, Attribute] | None = None, | |||
properties: dict[str, Attribute] | None = None, |
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.
Ad properties are not extensible, and would only contain the sym_name, I would just pass a StringAttr | None
instead, and create the properties dictionary in the init
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.
Makes sense. Done, thanks.
xdsl/dialects/builtin.py
Outdated
if "sym_name" in self.properties and isinstance( | ||
self.properties["sym_name"], StringAttr | ||
): | ||
printer.print(f" @{self.properties['sym_name'].data}") |
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.
if "sym_name" in self.properties and isinstance( | |
self.properties["sym_name"], StringAttr | |
): | |
printer.print(f" @{self.properties['sym_name'].data}") | |
if self.sym_name is not None: | |
printer.print(f" @{self.sym_name.data}") |
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.
Thank you!
I meant to use the assembly_format, but there's currently a circular dependency of the assembly format parser on builtin dialect so let's not go there. |
It seems that modules in xdsl already support sym_name attribute but we don't parse @-starting names.