fix(stdlib): Correctly promote numbers to bigints when left-shifting #1354
Conversation
stdlib/runtime/numbers.gr
Outdated
| @@ -1833,7 +1833,13 @@ export let (<<) = (x: Number, y: Number) => { | |||
| } else { | |||
| let xval = coerceNumberToWasmI64(x) | |||
| let yval = coerceNumberToWasmI64(y) | |||
| WasmI32.toGrain(reducedInteger(WasmI64.shl(xval, yval))): Number | |||
| if (WasmI64.leU(WasmI64.clz(xval), yval)) { | |||
There was a problem hiding this comment.
It looks like this fast path only works for positive numbers—is there any other quick check that'd also work for negative numbers?
There was a problem hiding this comment.
Heh, this actually has the opposite effect that you are imagining: clz of a negative number is zero, so we always promote to a big integer. If you think that's a problem, I can try to refine the condition a bit more, but I just tested this program, and it works as intended:
import BigInt from "bigint"
print(5 << 64)
print(-5 << 64)
print(BigInt.shl(-5t, 64l))
output:
92233720368547758080
-92233720368547758080
-92233720368547758080
There was a problem hiding this comment.
Sorry if I wasn't clear—I wasn't trying to say that this wasn't correct, just that exactly your point—clz of a negative number is always zero, so negative numbers never make it to the fast path. I think it'd be better to have a check that entered the fast path for both positive and negative numbers.
There was a problem hiding this comment.
@ospencer Got it. I think that the version I just pushed should do it.
There was a problem hiding this comment.
Please don't be too annoyed at me, but I like the version you originally did better than this one. && and || are implemented with branching, so now it's pretty expensive to do this check, almost to the point that it might be better to just skip the check altogether and always promote. I think I'm fine to have just the positive case have a chance to be fast-tracked.
Another option would be doing an abs of the value and then doing the clz—can do an abs in like 3 instructions so it's not too much more than your original version (but up to you).
|
@ospencer I did the |
Closes #1350.