Add lagmma#3215
Conversation
jan-wassenberg
left a comment
jan-wassenberg
left a comment
There was a problem hiding this comment.
Very nice! Just one tiny optimization, and a question: should we maybe call the user-facing function LogGamma, and the implementation Lgamma or LogGamma+Suffix? This might be more obvious. The comment can still mention that it corresponds to lgamma.
|
|
||
| // [1, 2): logGamma = (y-1)*(y-2)*MidPoly(y - 1.5) + acc. | ||
| const V t = Sub(y, kMidCenter); | ||
| const V zero_factors = Mul(Sub(y, kOne), Sub(y, kTwo)); |
There was a problem hiding this comment.
One of the sub could be fused away into MulSub, or preferably MulAdd(f, m, -a) which is more widely natively supported.
There was a problem hiding this comment.
good catch!
- updated to use
MulAddon FMA targets- used fallback on non FMA targets as the intermediate rounding error was relatively large where lgamma is around 0
jan-wassenberg
left a comment
jan-wassenberg
left a comment
There was a problem hiding this comment.
Nice :) Got one more optimization suggestion:
| const V low_poly = impl.LowPoly(d, Sub(w, kLowCenter)); | ||
| V low; | ||
| if constexpr (HWY_NATIVE_FMA) { | ||
| low = MulAdd(w, low_poly, Neg(low_poly)); |
There was a problem hiding this comment.
Rather than MulAdd + Neg, we are better off using MulSub because that is actually native on some platforms (x86). Same below.
Even better, we can change to q = Sub(kTwo, y), then NegMulAdd(y, q, q) which is native on more platforms.
2 participants
Implementing
lgammafrom wishlist.tgammaPR to useMaskedXorOr.Thanks for taking a look!