transforms: lower polynomials to arithmetic operations#5951
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5951 +/- ##
==========================================
+ Coverage 86.73% 86.74% +0.01%
==========================================
Files 425 426 +1
Lines 63539 63628 +89
Branches 7289 7293 +4
==========================================
+ Hits 55108 55196 +88
- Misses 6865 6866 +1
Partials 1566 1566 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
We can take a look at this once the polynomial dialect is merged |
7dae066 to
58ae869
Compare
2674c91 to
e6f7a53
Compare
| raise NotImplementedError( | ||
| f"polynomial.eval scheme {scheme.value!r} has no lowering" | ||
| ) |
There was a problem hiding this comment.
Can you please raise a PassFailedException instead and add a test for it so that we detect what to update when new cases are added?
| upper = op.domain_upper.value.data if op.domain_upper is not None else None | ||
|
|
||
| scheme = op.eval_scheme | ||
| if scheme is polynomial.EvalScheme.CLENSHAW: |
There was a problem hiding this comment.
feels like a good place to use a match statement instead
| def _float_constant( | ||
| value: float, | ||
| tp: AnyFloat | VectorType[AnyFloat] | TensorType[AnyFloat], | ||
| rewriter: PatternRewriter, | ||
| ) -> arith.ConstantOp: | ||
| """Create and insert a float constant, handling scalar/vector/tensor types.""" | ||
| if isa(tp, VectorType[AnyFloat]): | ||
| attr = DenseIntOrFPElementsAttr.from_list(tp, [value]) | ||
| elif isa(tp, TensorType[AnyFloat]): | ||
| attr = DenseIntOrFPElementsAttr.from_list(tp, [value]) | ||
| elif isa(tp, AnyFloat): | ||
| attr = FloatAttr(value, tp) | ||
| else: | ||
| raise TypeError(f"Unsupported type for float constant: {tp}") | ||
| return rewriter.insert(arith.ConstantOp(attr)) | ||
|
|
There was a problem hiding this comment.
this feels familiar, do we have code like this in the framework already? I think there was something similar in your exp to polynomial pass, is that right? I think we might want a helper for this in builtin with dedicated tests.
There was a problem hiding this comment.
Hm, what is kind of suboptimal is that the function returns an arithmetic.ConstantOp but the arithmetic dialect is lower than the builtin dialect in the dialect stack (arith imports builtin).
But I could make a function in builtin dialect that returns FloatAttr | DenseIntOrFPElementsAttr and then the function in each of the passes would just be this:
return rewriter.insert(arith.ConstantOp(float_constant_attr(value, tp)))
but it feels a bit awkward to define another function then in each pass
There was a problem hiding this comment.
Yeah sorry that's what I meant
There was a problem hiding this comment.
I still think we should do this first before merging this PR
alexarice
left a comment
There was a problem hiding this comment.
I haven't checked the mathematics, but overall looks fine to me.
| @@ -0,0 +1,111 @@ | |||
| // RUN: xdsl-opt -p polynomial-eval-to-arith %s | filecheck %s | |||
|
|
|||
| builtin.module { | |||
There was a problem hiding this comment.
Nit: There's no need to have the builtin.module op explicitly, and not having it means you don't have to indent the entire file.
| offset = -(upper + lower) / (upper - lower) | ||
| scale_op = _float_constant(scale, tp, rewriter) | ||
| offset_op = _float_constant(offset, tp, rewriter) | ||
| scaled = rewriter.insert(arith.MulfOp(x, scale_op.result)) |
There was a problem hiding this comment.
Not essential, but I wonder if this would all be a lot cleaner with an implicit builder?
| lower_affine.LowerAffinePass(), | ||
| convert_scf_to_riscv_scf.ConvertScfToRiscvPass(), | ||
| expand_math_to_polynomials.ExpandMathToPolynomialsPass(), | ||
| polynomial_eval_to_arith.PolynomialEvalToArithPass(), |
There was a problem hiding this comment.
This feels like it should be a separate change?
| pytest.fail( | ||
| f"EvalScheme.{scheme.name} has no dispatch branch in " | ||
| f"polynomial-eval-to-arith. Add a case for it in " | ||
| f"PolynomialEvalToArith.match_and_rewrite. ({e})" | ||
| ) |
There was a problem hiding this comment.
let's test this with lit/filecheck instead
Based on the specified polynomial family, evaluation interval, and scheme lower the polynomial approximation to arithmetic operations. Only supports Clenshaw's recurrence evaluation so far.