Enh : use llvm.powi to handle ** with integer type exponent for accurate precision#6191
Conversation
| exponent_type = base_type; | ||
| } else { | ||
| throw CodeGenError("Only [Integer , Real] exponent operator are supported.", | ||
| x.m_right->base.loc); |
There was a problem hiding this comment.
Where does this convert v^2 to mul(v, v)?
There was a problem hiding this comment.
since the main motive behind hard coding **2 into mul operation was that we want to preserve the the precision because we thought llvm only had llvm.pow(float,float) intrinsic function, I looked for llvm intrinsic that accepts an i32 exponent and I found llvm.powi function. so now we don't need to hard code ** into mul (for **2 and **3 only), unless we do it for sake for efficiency(though I think it won't matter).
There was a problem hiding this comment.
Should we be strict about the operands to be of matching types (by using cast if needed)?
assem2002
force-pushed
the
precision-with-pow
branch
from
848ee76 to
5ecaba8
Compare
|
I made some critical changes. It might need a review from the top again :). |
… or extend floating point values
…a compile-time binary operation of some Binop expression, if it's possible to be done. If it can't it will return a nullptr and the caller would know that the requested compile-time calculation isn't possible and the BinOp expression can't have a compile-time value
- `BinOp` is `**` - Base expression is `REAL` - Exponent expression in `INTEGER`
…e within `RealBinOp` expression to preserve precision.
…t may get casted while building ASR]
|
Let me know when the tests pass, then I'll review carefully again. |
assem2002
force-pushed
the
precision-with-pow
branch
from
bf1a729 to
3067ab0
Compare
assem2002
force-pushed
the
precision-with-pow
branch
from
3067ab0 to
91a6578
Compare
|
It's ready for review. |
| Level::Error, Stage::Semantic, {Label("", {loc})})); | ||
| throw SemanticAbort(); | ||
| } | ||
| return nullptr; |
There was a problem hiding this comment.
Can this mean that unimplemented combinations are silently ignored?
There was a problem hiding this comment.
I checked for the places using this function and returning nullptr fits nice.
The reciever of the function should be expr_t* value (the compile time value of the whole expression), If we can't create one, we return nullptr indicating that the whole expression doesn't have compile time value.
certik
left a comment
certik
left a comment
There was a problem hiding this comment.
I think that this is good. So it turned out that llvm already had a good support for integer powers, we just didn't use it before?
Yeah, that's right. |
…ccurate precision (lfortran#6191) * Chore : Fix `convert_kind` utility function to appropriately truncate or extend floating point values * Enh : Enhance `BinOpHelper` to be a function that attempts to craete a compile-time binary operation of some Binop expression, if it's possible to be done. If it can't it will return a nullptr and the caller would know that the requested compile-time calculation isn't possible and the BinOp expression can't have a compile-time value * Fix : Don't attempt to do a cast if : - `BinOp` is `**` - Base expression is `REAL` - Exponent expression in `INTEGER` * Enh : Use `llvm.powi` intrinsic function with `IntegerBinOp` * Fix : Use `llvm.powi` intrinsic function if exponent is `Integer` type within `RealBinOp` expression to preserve precision. * Minor : Handle exponent of type integer in WASM backend * Add and register integration test * Test : Enhance test by using precision tolerance instead of direct comparison * Add NOFAST label, as we're testing precision so we shouldn't optimize * LLVM_VERSION : Use `powi.f**` and `powi.f**.i32` based on llvm version * Warning : Remove real(8) exponent warning as it isn't that helpful * Minor : initialize pointer, to silent warnings * minor fix : use f64_mul with real of kind 8 * minor : Use `llvm.powi.f**` name with llvm_version 12" * tests: Update reference tests
2 participants
Fix #6150