api: use constraints to carry type information in pyrdl #2890
Conversation
superlopuh
requested review from
AntonLydike,
georgebisbas,
math-fehr,
PapyChacal and
webmiche
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2890 +/- ##
==========================================
- Coverage 89.93% 89.90% -0.03%
==========================================
Files 402 403 +1
Lines 50386 50413 +27
Branches 7786 7787 +1
==========================================
+ Hits 45313 45324 +11
- Misses 3847 3862 +15
- Partials 1226 1227 +1 ☔ View full report in Codecov by Sentry. |
xdsl/irdl/attributes.py
Outdated
| def constr( | ||
| irdl: GenericAttrConstraint[AttributeInvT] | AttributeInvT | type[AttributeInvT], | ||
| ) -> GenericAttrConstraint[AttributeInvT]: | ||
| return cast(GenericAttrConstraint[AttributeInvT], irdl_to_attr_constraint(irdl)) |
There was a problem hiding this comment.
can I has docstring to explain why it's necessary?
There was a problem hiding this comment.
Looks good to me, I think this is going to solve our typing issue.
I'm just sad we won't have operand_def(i32) anymore (and I understand why we could but don't want to allow this).
I think that we should also probably rename constr into is and eq? Just so this is much clearer to the user?
And also, thanks a lot for taking care of it 🦄 !
| def constr( | ||
| irdl: GenericAttrConstraint[AttributeInvT] | AttributeInvT | type[AttributeInvT], | ||
| ) -> GenericAttrConstraint[AttributeInvT]: | ||
| return cast(GenericAttrConstraint[AttributeInvT], irdl_to_attr_constraint(irdl)) |
There was a problem hiding this comment.
Is this a temporary thing? My guess is that irdl_to_attr_constraint should have the exact same signature at some point right?
There was a problem hiding this comment.
Actually I'll look into how bad things would get if I were to give it this signature now, maybe it won't be that big a diff.
There was a problem hiding this comment.
OK it's a necessary temporary thing, at least until the things that irdl_to_attr_constraint are a little more strongly typed than just being Any, which is the current case in a few places it's called.
docs/Toy/toy/dialects/toy.py
Outdated
|
|
||
| TensorTypeF64: TypeAlias = TensorType[Float64Type] | ||
| UnrankedTensorTypeF64: TypeAlias = UnrankedTensorType[Float64Type] | ||
| AnyTensorTypeF64: TypeAlias = TensorTypeF64 | UnrankedTensorTypeF64 | ||
| AnyTensorTypeF64Constr = constr(TensorTypeF64) | constr(UnrankedTensorTypeF64) |
There was a problem hiding this comment.
I'm not a big fan of having to not allow things like operand_def(TensorType[Float64Type]).
I feel that allowing operand_def(TensorType[Float64Type]) would be quite useful? It should be possible with just an overload on operand_def.
There was a problem hiding this comment.
I think it is allowed right now, it's the union that isn't, this is what this line provides.
xdsl/irdl/constraints.py
Outdated
| def __or__( | ||
| self, value: GenericAttrConstraint[AttributeInvT], / | ||
| ) -> AnyOf[AttributeCovT | AttributeInvT]: |
There was a problem hiding this comment.
| def __or__( | |
| self, value: GenericAttrConstraint[AttributeInvT], / | |
| ) -> AnyOf[AttributeCovT | AttributeInvT]: | |
| def __or__( | |
| self, value: GenericAttrConstraint[AttributeCovT2], / | |
| ) -> AnyOf[AttributeCovT | AttributeCovT2]: |
Both arguments should be covariant here I believe
PR 2/2 to make the Pylance errors in operation definitions go away.
The proposal is to introduce a
constrmentod to construct a constraint given a PyRDL type, add|and&operators on constraints to combine them, and to require a constraint instead of a type for operand, result, attribute, and property definitions.Pylance errors: 317 (-172)