-
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: Add default valued attributes #3305
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3305 +/- ##
==========================================
+ Coverage 90.01% 90.03% +0.01%
==========================================
Files 445 445
Lines 55965 56166 +201
Branches 5364 5390 +26
==========================================
+ Hits 50379 50571 +192
- Misses 4177 4179 +2
- Partials 1409 1416 +7 ☔ 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.
Looks great to me and the implementation seems to capture the nuances of the differences between optional and non-optional default values attributes/properties as far as I can tell. Just one test-case is missing for full branch coverage I believe.
parsed = Parser(ctx, "test.default").parse_operation() | ||
|
||
assert isinstance(parsed, DefaultOp) | ||
|
||
assert parsed.def_prop.value.data == 0 | ||
|
||
assert parsed.def_opt_prop is not None | ||
|
||
assert parsed.def_opt_prop.value.data == 1 |
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.
Can we also add a test here where we parse in the operation as a generic but don't specify in the dictionary an optioanl attribute/property?
In MLIR the auto-generated getter (say getDefOptProp
) should then still return True
, while getAttr("def_opt_propt")
would return null/missing
assert parsed.opt_attr.value.data == 1 | ||
|
||
|
||
def test_generic_accessors(): |
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 feel this test particularly but also the one above it should probably not live here, but am not sure exactly where they should go
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.
Moving it to test_operation_definition.py
Also added some overloads to |
I hit a bug with renamed properties when continuing to update mlir. Should hopefully be fixed in latest commit |
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.
Very cool!
I find ODS semantics quite weird, I didn't know you could have optional attributes with default values, I think it completely breaks the roundtripping.
Thanks for looking at it and implementing it!
Adds the equivalents for the tablegen `DefaultValuedAttribute` and `DefaultValuedOptionalAttribute`.
Adds the equivalents for the tablegen
DefaultValuedAttribute
andDefaultValuedOptionalAttribute
.This is a blocker for updating mlir.
@zero9178, would you be able to check if the test cases agree with what you think mlir should be doing, as you seemed to have some idea about what was going on here when I last asked about this.
Lastly is there a nice python way to specify that an
opt_prop_def
should return anAttribute
instead ofAttribute | None
when it is specified with a default value?