dialects: (complex) add folding canonicalization pattern#5497
dialects: (complex) add folding canonicalization pattern#5497radosavkrunic33 wants to merge 2 commits intoxdslproject:mainfrom
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5497 +/- ##
=======================================
Coverage 86.18% 86.19%
=======================================
Files 399 399
Lines 56570 56627 +57
Branches 6510 6518 +8
=======================================
+ Hits 48754 48808 +54
- Misses 6282 6296 +14
+ Partials 1534 1523 -11 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Hi @radosavkrunic33, sorry about the delay, we finally merged that branch, let's revisit this when you have time? |
|
Yes, I'll do it. Thank you for reminding me. |
It folds complex floating point binary ops whose operands are both s The complex binary ops that support folding are add, sub, mul, div
b92535e to
087cc60
Compare
| return cast( | ||
| Operation, | ||
| ConstantOp.build(properties={"value": value}, result_types=(type,)), | ||
| ) |
There was a problem hiding this comment.
What's the error if you remove the cast?
There was a problem hiding this comment.
There is no error :)
There was a problem hiding this comment.
Great, I would have been surprised, let's remove it in that case?
| if im_lhs == 0.0: | ||
| imag = float("nan") | ||
| else: | ||
| if math.copysign(im_lhs, im_rhs) > 0: | ||
| positive = True if im_lhs > 0 else False | ||
| else: | ||
| positive = True if im_lhs < 0 else False | ||
| imag = float("inf") if positive else float("-inf") |
There was a problem hiding this comment.
This is all unexpected and worth a comment IMO. Where are these results defined? I would have expected division by 0 to be undefined by default.
If we are sure that this is the expected result, it seems a bit more straightforward to define it this way, what do you think?
| if im_lhs == 0.0: | |
| imag = float("nan") | |
| else: | |
| if math.copysign(im_lhs, im_rhs) > 0: | |
| positive = True if im_lhs > 0 else False | |
| else: | |
| positive = True if im_lhs < 0 else False | |
| imag = float("inf") if positive else float("-inf") | |
| inf = float("inf") # or _INF at top of file maybe, same for nan | |
| if im_lhs == 0.0: | |
| imag = float("nan") | |
| else: | |
| # Positive infinity if signs match, negative otherwise | |
| imag = math.copysign(inf, im_lhs) / math.copysign(1.0, im_rhs) |
There was a problem hiding this comment.
For integer, division by 0 is undefined behavior. For floating-point division, the IEEE‑754 standard defines the behavior-https://en.wikipedia.org/wiki/Division_by_zero#:~:text=Floating-point%20arithmetic,-In%20computing%2C%20most&text=A%20NaN%20(not%20a%20number,same%20sign%20as%20the%20dividend. .MLIR follows IEEE754 specification for floating-point arithmetic. Please, correct me if I'm wrong. A related discussion can be found here.
superlopuh
left a comment
There was a problem hiding this comment.
Can you please add a lit test for each of the patterns added?
|
I tested all these patterns locally by setting When folding is enabled for all canonicalization patterns and all lit tests are run, an issue occurs with |
|
I think that would make sense, and I'm not sure what the expected behaviour of adding two Trues is. @math-fehr ? |
|
|
|
@osmanyasar05, I'll try to enable folding soon and get back to you on this, I just ran this locally and there are some more errors that shouldn't be fixed in this PR anyway. |
This PR folds complex floating point binary ops whose operands are both
complex.constants.The complex binary ops that support folding are
add,sub,mul,div.It's a split-out PR from @#5440.