transformations: Canonicalizations for arith on float constants #3362
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3362 +/- ##
==========================================
+ Coverage 90.09% 90.10% +0.01%
==========================================
Files 447 449 +2
Lines 56578 56666 +88
Branches 5431 5440 +9
==========================================
+ Hits 50975 51060 +85
- Misses 4166 4168 +2
- Partials 1437 1438 +1 ☔ View full report in Codecov by Sentry. |
|
Ok, happy to have them in separate PRs! |
| case arith.Divf: | ||
| val = l.value.data / r.value.data |
There was a problem hiding this comment.
What happens if val is NaN?
We should probably check for it and only do the replacement if val is not NaN, as arith.constant doesn't support NaN afaik?
There was a problem hiding this comment.
Excellent point. Theoretically, python should handle it. For instance, 3 / float('nan') will evaluate to nan.
However, I can't seem to get nan to round-trip to verify this. str(builtin.FloatAttr(float('nan'), 32)) returns 'nan : f32', but arith.constant nan : f32 will throw a parser error saying:
%a = arith.constant nan : f32
^^^
attribute expected
Am I doing this right?
There was a problem hiding this comment.
can mlir parse that? it's possible that xDSL has a bug here
There was a problem hiding this comment.
mlir will say
<source>:6:24: error: expected attribute value
%b = arith.constant nan : f32
^
Compiler returned: 1
However, it appears that mlir prints and parses nan/inf as hexadecimals, whereas we would print it as nan : f32 / inf : f32 which correctly fails to parse.
There was a problem hiding this comment.
Do you have the bandwidth to fix xDSL printing of nans in that case?
There was a problem hiding this comment.
It's not straightforward, xdsl likely parses hexadecimal float literals as hexadecimal int literals, from which it builds a float rather than using it as a bit-string. See the floatattr docs
| @@ -15,3 +15,22 @@ | |||
| // CHECK-NEXT: %addf = arith.addf %lhsf32, %rhsf32 : f32 | |||
| // CHECK-NEXT: %addf_vector = arith.addf %lhsvec, %rhsvec : vector<4xf32> | |||
| // CHECK-NEXT: "test.op"(%addf, %addf_vector) : (f32, vector<4xf32>) -> () | |||
|
|
|||
There was a problem hiding this comment.
Nit: I believe we prefer to put filecheck tests above the test now? If so this file is small enough to fix imo
alexarice
changed the title
This adds the second of two canonicalisation patterns for `FloatingPointLikeBinaryOperation`s on `arith.constants`. * Ops with two constant operands are folded by canonicalisation, regardless of math flags (see #3362 ) * Chained ops of the form `(const1 * var) * const2` are rewritten as `(const1 * const2) * var` iff the `fastmath<reassoc>` or `fastmath<fast>` flags are set, and if the ops are multiplications or additions. Note, this is not currently implemented in upstream mlir. See [here](https://godbolt.org/z/1KKf6vbfv) to check how it works in upstream mlir. Note, these canonicalisations currently do not support container types. --------- Co-authored-by: n-io <n-io@users.noreply.github.com>
…project#3362) This adds the first of two canonicalisation patterns `FloatingPointLikeBinaryOperation`s on `arith.constants`. * Ops with two constant operands are folded by canonicalisation, regardless of math flags * ~~Chained ops of the form `(const1 * var) * const2` are rewritten as `(const1 * const2) * var` iff the `fastmath<reassoc>` or `fastmath<fast>` flags are set, and if the ops are multiplications or additions. Note, this is not currently implemented in upstream mlir.~~ See [here](https://godbolt.org/z/1KKf6vbfv) to check how it works in upstream mlir. Note, these canonicalisations currently do not support container types. --------- Co-authored-by: n-io <n-io@users.noreply.github.com>
…t#3364) This adds the second of two canonicalisation patterns for `FloatingPointLikeBinaryOperation`s on `arith.constants`. * Ops with two constant operands are folded by canonicalisation, regardless of math flags (see xdslproject#3362 ) * Chained ops of the form `(const1 * var) * const2` are rewritten as `(const1 * const2) * var` iff the `fastmath<reassoc>` or `fastmath<fast>` flags are set, and if the ops are multiplications or additions. Note, this is not currently implemented in upstream mlir. See [here](https://godbolt.org/z/1KKf6vbfv) to check how it works in upstream mlir. Note, these canonicalisations currently do not support container types. --------- Co-authored-by: n-io <n-io@users.noreply.github.com>
This adds the first of two canonicalisation patterns
FloatingPointLikeBinaryOperations onarith.constants.Chained ops of the form(const1 * var) * const2are rewritten as(const1 * const2) * variff thefastmath<reassoc>orfastmath<fast>flags are set, and if the ops are multiplications or additions. Note, this is not currently implemented in upstream mlir.See here to check how it works in upstream mlir.
Note, these canonicalisations currently do not support container types.