-
-
Notifications
You must be signed in to change notification settings - Fork 679
Improve Math routines + reinmplement Mathf.random #297
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
As far as I understood |
|
EDIT SSE/AVX can simulate cmov via masking and blend both arguments but as I mentioned before that not cheap |
std/assembly/math.ts
Outdated
| let z_h = cp_h * p_h; | ||
| let dp_l = select<f64>(dp_l1, 0.0, k); | ||
| // let dp_l = select<f64>(dp_l1, 0.0, k); | ||
| let dp_l: f64 = dp_l1 * <f64><bool>k; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the : f64 annotation isn't necessary because 1.0 and dp_l1 are already f64. <bool>k will compile to i32.and(k, 1) if I'm not mistaken, while (k != 0) is an i32.ne(k, 0), hmm. Maybe (k != 0) is easier to understand.
std/assembly/math.ts
Outdated
| return y; | ||
| export function round(x: f64): f64 { | ||
| if (!isFinite(x) || x == 0) return x; | ||
| if (-0.5 <= x && x < 0) return -0.0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe copysign(0, x)? and remove the x == 0 check? Overall this looks like it has 4 branches
std/assembly/math.ts
Outdated
| let z = builtin_sqrt<f64>(yy + 1); | ||
| if (e >= 0x3FF + 1) y = log(2 * y + 1 / (z + y)); | ||
| else if (e >= 0x3FF - 26) y = log1p(y + yy / (z + 1)); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This refactor doesn't seem to be worth it because it now calculates yy and z even if none of the if conditions is true
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, you are right
std/assembly/math.ts
Outdated
| var twopk = reinterpret<f64>(u); | ||
| var y: f64; | ||
| if (k < 0 || k > 56) { | ||
| if (<i32>(k < 0) | <i32>(k > 56)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should make the compiler smarter about logical ors instead of convoluting the source like this. Wdyt?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Definitely yes! I even have opened proposal for that: #277
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, can we keep the || here and at the other places, if any, for now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sure
std/assembly/math.ts
Outdated
| if (ey == 0x7FF) return y; | ||
| x = reinterpret<f64>(ux); | ||
| if (ex == 0x7FF || uy == 0) return x; | ||
| if (<i32>(ex == 0x7FF) | <i32>(uy == 0)) return x; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Like here
std/assembly/math.ts
Outdated
| var hx = <u32>(u >> 32); | ||
| var k = 0; | ||
| if (hx < 0x00100000 || <bool>(hx >> 31)) { | ||
| if (<u32>(hx < 0x00100000) | (hx >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var hx = <u32>(u >> 32); | ||
| var k = 0; | ||
| if (hx < 0x00100000 || <bool>(hx >> 31)) { | ||
| if (<u32>(hx < 0x00100000) | (hx >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var k = 1; | ||
| var c = 0.0, f = 0.0; | ||
| if (hx < 0x3FDA827A || <bool>(hx >> 31)) { | ||
| if (<u32>(hx < 0x3FDA827A) | (hx >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var hx = <u32>(u >> 32); | ||
| var k = 0; | ||
| if (hx < 0x00100000 || <bool>(hx >> 31)) { | ||
| if (<u32>(hx < 0x00100000) | hx >> 31) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
| k = <i32>(invln2 * x + builtin_copysign<f32>(0.5, x)); | ||
| } else { | ||
| k = 1 - sign_ - sign_; | ||
| k = 1 - (sign_ << 1); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this improve something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think yes because sign_ - sign_ is two reads from locals, sign_ << 1 only one read operation
std/assembly/math.ts
Outdated
| var twopk = reinterpret<f32>(u); | ||
| var y: f32; | ||
| if (k < 0 || k > 56) { | ||
| if (<i32>(k < 0) | <i32>(k > 56)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var u = reinterpret<u32>(x); | ||
| var k = 0; | ||
| if (u < 0x00800000 || <bool>(u >> 31)) { | ||
| if (<u32>(u < 0x00800000) | (u >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var ix = reinterpret<u32>(x); | ||
| var k = 0; | ||
| if (ix < 0x00800000 || <bool>(ix >> 31)) { | ||
| if (<u32>(ix < 0x00800000) | (ix >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var c: f32 = 0, f: f32 = 0; | ||
| var k: i32 = 1; | ||
| if (ix < 0x3ED413D0 || <bool>(ix >> 31)) { | ||
| if (<u32>(ix < 0x3ED413D0) | (ix >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| var ix = reinterpret<u32>(x); | ||
| var k: i32 = 0; | ||
| if (ix < 0x00800000 || <bool>(ix >> 31)) { | ||
| if (<u32>(ix < 0x00800000) | (ix >> 31)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
std/assembly/math.ts
Outdated
| if (iy == 0) return 1.0; // x**0 = 1, even if x is NaN | ||
| // if (hx == 0x3F800000) return 1.0; // C: 1**y = 1, even if y is NaN, JS: NaN | ||
| if (ix > 0x7F800000 || iy > 0x7F800000) return x + y; // NaN if either arg is NaN | ||
| if (<i32>(ix > 0x7F800000) | <i32>(iy > 0x7F800000)) return x + y; // NaN if either arg is NaN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here
| y *= Ox1p_126f; | ||
| n += 126; | ||
| y *= Ox1p_126f * Ox1p24f; | ||
| n += 126 - 24; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's this doing?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See original code:
https://git.musl-libc.org/cgit/musl/tree/src/math/scalbnf.c
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It seems this fix appear later in original source code. Ported "musl" don't reflect this changes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, updates, nice :)
| if (ux << 1 == 0) return x; | ||
| if (!ex) { | ||
| for (i = uxi << 9; i >> 31 == 0; ex--, i <<= 1) {} | ||
| ex -= builtin_clz<u32>(uxi << 9); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice find :)
| if (u < 0x3F800000 - (12 << 23)) return 1; | ||
| let t = expm1(x); | ||
| return 1 + t * t / (2 * (1 + t)); | ||
| return 1 + t * t / (2 + 2 * t); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also add a comment here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
| if (u < 0x42B17217) { | ||
| let t = exp(x); | ||
| return 0.5 * (t + 1 / t); | ||
| return 0.5 * t + 0.5 / t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe also add a comment here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yeah
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done!
|
Great, thanks! :) |
Math.roundxoroshiro64**copysignif possibleavoidingselect<f64/f32>because it produce complex branchless machine code unlike integer version (which usecmovinstructions)Closes #59