dialects: (csl) adding constructors for CSL ops #2742
Conversation
n-io
added
dialects
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #2742 +/- ##
==========================================
- Coverage 89.93% 89.91% -0.03%
==========================================
Files 367 367
Lines 47282 47302 +20
Branches 7219 7229 +10
==========================================
+ Hits 42525 42531 +6
- Misses 3645 3659 +14
Partials 1112 1112 ☔ View full report in Codecov by Sentry. |
| fname: str, | ||
| result_type: Attribute, | ||
| struct: Operation, | ||
| *params: SSAValue | Operation, |
There was a problem hiding this comment.
I think I'd like this to not be a vararg, and instead be a Sequence[SSAValue | Operation].
Having a vararg after a bunch of other arguments would make the code less readable at the call site imo.
There was a problem hiding this comment.
As long as it's limited to function name+type, it should hopefully be fine enough to avoid me doing cross-PR changes (:
There was a problem hiding this comment.
I'm strongly in favour of switching to Sequence here, as it's otherwise out of line with the other call operations...
| @@ -1486,6 +1533,13 @@ class ParamOp(IRDLOperation): | |||
|
|
|||
| res = result_def(T) | |||
|
|
|||
| def __init__(self, name: str, result_type: T): | |||
There was a problem hiding this comment.
I'm not sure if this is allowed in python... @PapyChacal do you have any input on this? I think you know more on this than I do...
There was a problem hiding this comment.
I thought that's what make TypeVars so neat
https://mypy.readthedocs.io/en/stable/generics.html
There was a problem hiding this comment.
But T is not a TypeVar, it's an IRDL constraint! These two are sadly quite different...
There was a problem hiding this comment.
I don't think it's necessarily wrong, but it's not expressing what you may think it is.
There was a problem hiding this comment.
You're right, I misread, it's not a TypeVar. I think it's not an IRDL constraint either, though, it should just be a union type with some additional meta data.
xdsl/dialects/csl.py
Outdated
| def __init__(self, op: SSAValue | Operation): | ||
| typ = op.results[0].type if isinstance(op, Operation) else op.type | ||
| assert isinstance(typ, IntegerType) | ||
| res_signedness = ( | ||
| Signedness.SIGNLESS | ||
| if typ.signedness.data == Signedness.UNSIGNED | ||
| else Signedness.UNSIGNED | ||
| ) | ||
| super().__init__( | ||
| operands=[op], result_types=[IntegerType(typ.width, res_signedness)] | ||
| ) |
There was a problem hiding this comment.
I think I'd prefer a much simpler interface that is in-line with other cast operations:
| def __init__(self, op: SSAValue | Operation): | |
| typ = op.results[0].type if isinstance(op, Operation) else op.type | |
| assert isinstance(typ, IntegerType) | |
| res_signedness = ( | |
| Signedness.SIGNLESS | |
| if typ.signedness.data == Signedness.UNSIGNED | |
| else Signedness.UNSIGNED | |
| ) | |
| super().__init__( | |
| operands=[op], result_types=[IntegerType(typ.width, res_signedness)] | |
| ) | |
| def __init__(self, op: SSAValue | Operation, result_type: IntegerType): | |
| super().__init__( | |
| operands=[op], result_types=[result_type] | |
| ) |
This way the caller can decide what he wants instead of the init smartly guessing.
There was a problem hiding this comment.
If you prefer a simple interface, I suggest it's simplest to do what the op promises to do, and just flip the signedness.
There was a problem hiding this comment.
But you may want to cast from i32 to si32, which this constructor does not allow.
There was a problem hiding this comment.
I understood we didn't want to have si32, but I added an optional arg to explicitly state the result type, and if it's None we derive it.
|
A very nice set of changes, I just have a bunch of nitpicks strewn about :D |
Fix some of the things that accidentally went through in #2742. Also moves the `__init__` to the top of the method definition and adds some docstrings.
Adding constructors for many CSL ops.
This was originally part of #2720