Implement missing math functions in LLVM backend #739
Conversation
There was a problem hiding this comment.
Hi @mattisboeckle, thanks for the great first contribution!
I have a few maintenance discussion topics below -- even though the contribution is great and already mergeable, it would be nice to make sure that the definitions we want to use are maintainable in the future. I'm interested in your opinion as to how we should act regarding the more complex functions / constants that are not available directly as intrinsics before we proceed :)
| // We have to use the non-intrinsic version of 'atan' as it was only added in LLVM 19 | ||
| llvm """ | ||
| %i = call %Double @atan(double 1.0) | ||
| %z = fmul %Double %i, 4.0 | ||
| ret %Double %z | ||
| """ |
There was a problem hiding this comment.
Just to double-check, will LLVM optimise this to a simple constant when used? 🤔 (that's somewhat important when working with π).
We could also:
- just put in the proper
3.14....constant here as best as we can - instead of the Chez and LLVM definitions, have
default { tan(1.0) * 4.0 }or something like that (perhapsdefaultcan only take a fn call, not sure?
WDYT?
There was a problem hiding this comment.
Just checked in a small test file. LLVM will optimize this function to a constant.
The other options work as well, not sure which I prefer
| llvm """ | ||
| %i = fadd %Double 1.0, ${x} | ||
| %z = call %Double @llvm.log.f64(double %i) ret %Double %z | ||
| """ |
There was a problem hiding this comment.
FWIW, as far as I remember, log1p is in libc as a separate function for numerical precision reasons. I know MLIR has it as a separate intrinsic, but I don't know about LLVM.
I'd either use either that ^ or just with a default { log(1.0 + x) } instead of the Chez and LLVM externs? WDYT?
EDIT: the musl source seems to quote a textbook showing how to compute log1p given a very accurate log: https://git.musl-libc.org/cgit/musl/tree/src/math/log1p.c#n46, but it seems like a very strong precondition to me 🤔
There was a problem hiding this comment.
log1p does not appear in the LLVM docs for intrinsics, so I defaulted to this
examples/pos/math.effekt
Outdated
| def main() = { | ||
| println(cos(0.0)); | ||
| println(sin(0.0)); | ||
| println(log(1.0)); | ||
| println(log1p(0.0)); | ||
| println(exp(0.0)); | ||
| println(pow(2.0, 5.0)); | ||
| println(tan(0.0)); | ||
| println(atan(0.0)); | ||
| } |
There was a problem hiding this comment.
Nitpick: we tend to use two spaces indentation when formatting Effekt files (and also the semicolons are not needed, but we're not that strict about them). :)
It also looks like there's no test for pi -- would something like sin(pi / 2) == 1 help as a test?
There was a problem hiding this comment.
Nitpick accepted. Although I did copy the doubles.effekt file which uses 4 spaces 🤷
Implement cos, sin, atan, tan, log, log1p, _pi, exp & pow. All functions use the LLVM intrinsics except tan & atan. They do not exist in the version of LLVM we are using. Fixes effekt-lang#608
mattisboeckle
force-pushed
the
fix/llvm-missing-math
branch
from
3979a04 to
1dfcaf1
Compare
Implement cos, sin, atan, tan, log, log1p, _pi, exp & pow. All functions use the LLVM intrinsics except tan & atan. They do not exist in the version of LLVM we are using.
Fixes #608