dialects: (arith) add SignlessIntegerBinaryOperation canonicalization #3583
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3583 +/- ##
==========================================
- Coverage 90.74% 90.71% -0.03%
==========================================
Files 465 465
Lines 58755 58813 +58
Branches 5630 5630
==========================================
+ Hits 53317 53354 +37
- Misses 3997 4016 +19
- Partials 1441 1443 +2 ☔ View full report in Codecov by Sentry. |
alexarice
changed the title
xdsl/dialects/arith.py
Outdated
| @staticmethod | ||
| def is_right_unit(attr: AnyIntegerAttr) -> bool: | ||
| return False |
There was a problem hiding this comment.
Can you add documentation on that function?
Why is there a right in the name? It doesn't seem that it takes the left/right side into account?
There was a problem hiding this comment.
i being a right unit means that x . i = x but we don't necessarily have i . x = x. See the shift operations for example
| @staticmethod | ||
| def is_right_unit(attr: AnyIntegerAttr) -> bool: | ||
| return attr == IntegerAttr(1, attr.type) |
There was a problem hiding this comment.
Why is this one taking the type into account, but the one in Addi just checks the value?
There was a problem hiding this comment.
The one here is just to work around that the unit for i1 multiplication is -1 and not 1. This doesn't matter in the 0 case which Addi has
There was a problem hiding this comment.
Thinking about it I'm not 100% sure this will work for si1
There was a problem hiding this comment.
@superlopuh is there a better way to do this check?
|
Btw, at some point we should define an interface for |
|
|
|
|
|
I've realised that being a zero for a function might not be common terminology for non-mathematicians. Would it be preferable to rename this to |
|
Both of those seem more confusing 😅 |
|
I've left the names the same but added docstrings. @math-fehr @superlopuh does this make sense? |
| def is_right_zero(attr: AnyIntegerAttr) -> bool: | ||
| """ | ||
| Returns True only when 'attr' is a right zero for the operation | ||
| https://en.wikipedia.org/wiki/Absorbing_element | ||
|
|
||
| Note that this depends on the operation and does *not* imply that | ||
| attr.value.data == 0 | ||
| """ | ||
| return False | ||
|
|
||
| @staticmethod | ||
| def is_right_unit(attr: AnyIntegerAttr) -> bool: | ||
| """ | ||
| Return True only when 'attr' is a right unit/identity for the operation | ||
| https://en.wikipedia.org/wiki/Identity_element |
There was a problem hiding this comment.
ooh, if there are wikipedia pages with those titles then maybe using those for function names is fair game
There was a problem hiding this comment.
the wikipedia page for Absorbing element says that Absorbing element/annihilating element/zero are also used in various contexts
alexarice
force-pushed
the
alexarice/arith-canon
branch
from
d883932 to
6e484a7
Compare
|
This suffers from a similar problem to #3585 . I'll try to work out the proper overflow semantics |
alexarice
force-pushed
the
alexarice/arith-canon
branch
from
6e484a7 to
bf6a5fa
Compare
|
I've rebased, added an |
xdsl/dialects/arith.py
Outdated
| value: int | IntAttr, | ||
| value_type: int | IntegerType | IndexType, | ||
| *, | ||
| truncate_bits: bool = False, | ||
| ) -> ConstantOp: | ||
| if isinstance(value_type, int): | ||
| value_type = IntegerType(value_type) | ||
| return ConstantOp.create( | ||
| result_types=[value_type], | ||
| properties={"value": IntegerAttr(value, value_type)}, | ||
| properties={ | ||
| "value": IntegerAttr(value, value_type, truncate_bits=truncate_bits) | ||
| }, |
There was a problem hiding this comment.
how do you feel about a quick separate PR for this guy?
|
Should be ready for review again |
Got bored of implementing various arith canonicalizations that all looked the same, so I've done a bunch of them generically in one go.
I've missed out a fair few methods (shifting operations in particular stand out) where I wasn't sure if the python operation agreed with the mlir operation.