dialects: (arith) split generic binary op definition into specific ones #3274
Conversation
| @@ -214,10 +213,15 @@ def __hash__(self) -> int: | |||
| return id(self) | |||
|
|
|||
|
|
|||
| SignlessIntegerBinaryOp = BinaryOperation[Annotated[Attribute, signlessIntegerLike]] | |||
| class FloatingPointLikeBinaryOperation(IRDLOperation, abc.ABC): | |||
There was a problem hiding this comment.
turns out all the floating ops below are actually with fastmath
| Annotated[Attribute, floatingPointLike] | ||
| ] | ||
|
|
||
| IntegerBinaryOp = BinaryOperation[IntegerType] |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3274 +/- ##
=======================================
Coverage 89.98% 89.98%
=======================================
Files 444 444
Lines 55628 55631 +3
Branches 5352 5353 +1
=======================================
+ Hits 50057 50060 +3
Misses 4174 4174
Partials 1397 1397 ☔ View full report in Codecov by Sentry. |
| @classmethod | ||
| def constr( | ||
| cls, | ||
| *, | ||
| n: GenericAttrConstraint[Attribute] | None = None, | ||
| p: GenericAttrConstraint[_T] | None = None, | ||
| q: GenericAttrConstraint[Attribute] | None = None, | ||
| ) -> BaseAttr[ParamOne[Attribute]] | ParamAttrConstraint[ParamOne[_T]]: | ||
| if n is None and p is None and q is None: | ||
| return BaseAttr(cls) | ||
| return ParamAttrConstraint(cls, (n, p, q)) | ||
|
|
There was a problem hiding this comment.
Are those changes related?
| @@ -271,7 +273,7 @@ def get_canonicalization_patterns(cls) -> tuple[RewritePattern, ...]: | |||
|
|
|||
|
|
|||
| @irdl_op_definition | |||
| class Addi(SignlessIntegerBinaryOp): | |||
| class Addi(SignlessIntegerBinaryOperation): | |||
There was a problem hiding this comment.
Any particular reason to change the name here?
There was a problem hiding this comment.
The undocumented recommendation is for ABCs to have the full Operation and concrete ops to have Op as a suffix
| @@ -15,7 +15,8 @@ | |||
|
|
|||
| # map the arith operation to the right varith op: | |||
| ARITH_TO_VARITH_TYPE_MAP: dict[ | |||
| type[arith.BinaryOperation[Attribute]], type[varith.VarithOp] | |||
| type[arith.SignlessIntegerBinaryOperation | arith.FloatingPointLikeBinaryOperation], | |||
There was a problem hiding this comment.
it is worth making a type alias for this?
There was a problem hiding this comment.
IDK, I wouldn't bother
|
I can't even see the changes Mathieu reviewed, was this just a merging/rebasing mishap? |
Co-authored-by: Alex Rice <alexrice999@hotmail.co.uk>
This makes the next step for Pyright updating easier, as we can't actually properly represent generics in the IRDL constraint system. Currently, the arith base class helpers work because the generics are all specified by the time the ops are "defined" with the annotation. But my proposed solution of replacing the Annotated with VarConstraints means that we need to define the constraint on a specific type already. My understanding is that this is not a functional change, only requiring some clients of the base classes to update the names that they refer to.
Part of #3264
Note stacked PR.