misc: Implement and test dynamic IRDL dialect registering. #2654
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2654 +/- ##
==========================================
+ Coverage 89.63% 89.64% +0.01%
==========================================
Files 362 364 +2
Lines 46341 46546 +205
Branches 7009 7033 +24
==========================================
+ Hits 41537 41728 +191
- Misses 3726 3733 +7
- Partials 1078 1085 +7 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Very cool to see this working!
I have some comments on the structure of the interpreter, but otherwise that looks good to me!
In term of testing, I'm wondering if we should not just add this feature to xdsl-opt directly, so we can test it with xdsl-opt --load-irdl="my_file.irdl" my_file.mlir?
xdsl/interpreters/irdl.py
Outdated
| for k, v in get_accessors_from_param_attr_def( | ||
| self.attrs_defs[self.current] | ||
| ).items(): | ||
| setattr(self.attrs[op.sym_name], k, v) | ||
| setattr( | ||
| self.attrs[op.sym_name], "name", f"{self.dialect_name}.{op.sym_name.data}" | ||
| ) |
There was a problem hiding this comment.
I feel that this should be moved somewhere else?
We should probably have a function in irdl.py that just does all the initialization for an attribute that is being defined? So if we ever refactor something we won't forget this place if that makes sense?
There was a problem hiding this comment.
Reminder; This is at the core because EqAttr expect an existing Attribute type.
Because of cyclic references, my first iteration is to first define shallow Attribute types and progressively populate them.
Indeed, a probably cleaner approach would be to first work with more abstract references, and resolve them later.
Do we want to work on this now or on a second PR? Genuine question, I'm not looking forward to this working as much as you AFAIU 😋 So your call!
There was a problem hiding this comment.
BUt I might have overinterpreted; I guess literally those lines could be factored out, right?
There was a problem hiding this comment.
Thing is, injecting it directly in the type's dictionary is specific to this compromise in the dynamic Dialect creation: IRDL typically creates the full dict and then give it to the type constructor.
We could switch both to this style to duplicate 🤷
There was a problem hiding this comment.
I rewrote it to get the IRDL code out of the loop and do:
attr = self.get_attr(interpreter, name)
attr_def = self._get_attr_def(interpreter, name)
to_inject = get_accessors_from_param_attr_def(attr_def)
for k, v in to_inject.items():
setattr(attr, k, v)doest it help? It clarifies that the only added thing from already IRDL-factored code is the late injection in the type's dictionary.
I'd love a clean attr.__dict__ |= to_inject, but that is forbidden wizardry 🧙
xdsl/interpreters/irdl.py
Outdated
| for k, v in get_accessors_from_op_def( | ||
| self.ops_defs[self.current], None | ||
| ).items(): | ||
| setattr(self.ops[op.sym_name], k, v) | ||
| setattr( | ||
| self.ops[op.sym_name], "name", f"{self.dialect_name}.{op.sym_name.data}" | ||
| ) |
There was a problem hiding this comment.
Tried something on the other comment, will replicate whatever satisfying answer here.
|
Concretely, I think it would be great to have separate unit tests for the name helpers and for set_data on the interpreter. |
|
Looks good now with tests |
This is an implementation - through our interpreter infrastructure - of IRDL dialect registering. That is, at runtime, read an IRDL dialect in MLIR form, and register the described dialect to use right away.
NB: This only implements the steps described here; avoiding any more involved discussion abut how exactly to integrate this in
xdsl-optor hack in Python import mechanism and what not. To be continued!