Skip to content
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

add toBits and fromBits to math #16403

Closed
wants to merge 2 commits into from
Closed

Conversation

ringabout
Copy link
Member

@ringabout ringabout commented Dec 19, 2020

It's not easy to support JS backend.

Todo in the following PR:

  • signbits
  • copysign

@Clyybber
Copy link
Contributor

Clyybber commented Dec 19, 2020

I don't think this should be in math.nim or in the stdlib at all because I don't see how it's better than writing the cast yourself.

@ringabout
Copy link
Member Author

ringabout commented Dec 19, 2020

It's the basis of signbits and copysign function(Though I could not export them or use C ffi instead)
signbits and copysign are C99 function which are not in Nim yet. (copysign/signbits exists in many programming languages such as C/C++, Golang, Crystal, Zig, D etc)

As to toBits/fromBits

@bitCast in Zig

Float64bits in GoLang/VLang
https://golang.org/src/math/unsafe.go

toBits in Rust
https://doc.rust-lang.org/std/primitive.f64.html#method.to_bits

@ringabout ringabout marked this pull request as draft December 19, 2020 13:37
@ringabout ringabout marked this pull request as ready for review December 19, 2020 13:55
@ringabout
Copy link
Member Author

ringabout commented Dec 19, 2020

So which is better?

func toBits*(x: float64): uint64 =
  ## Returns the IEEE 754 binary representation of float64.
  cast[ptr uint64](unsafeAddr x)[]

func toBits2*(x: float64): uint64 =
  ## Returns the IEEE 754 binary representation of float64.
  cast[uint64](x)

echo toBits(12.4)
echo toBits2(12.4)
N_LIB_PRIVATE N_NIMCALL(NU64, toBits)(NF x) {
	NU64 result;
	result = (NU64)0;
	result = (*((NU64*) ((&x))));
	return result;
}
N_LIB_PRIVATE N_NIMCALL(NU64, toBits2)(NF x) {
	NU64 result;
	union { NF source; NU64 dest; } LOC1;
	result = (NU64)0;
	LOC1.source = x;
	result = LOC1.dest;
	return result;
}

I prefer cast which looks more clear.

@ringabout ringabout closed this Dec 20, 2020
@timotheecour
Copy link
Member

@xflywind

So which is better?

can you check generated asm in both cases? if cast[ptr uint64](unsafeAddr x)[] leads to better/more performant code than cast[uint64](x) then maybe there's a case for adding toBits

@timotheecour timotheecour added the stale Staled PR/issues; remove the label after fixing them label Jan 2, 2021
@ringabout
Copy link
Member Author

ringabout commented Jan 2, 2021

@timotheecour

cast[ptr uint64](unsafeAddr x)[]

  • Error: opcCastIntToPtr: regs[rb].kind: rkNodeAddr

This doesn't work in VM
but cast[uint64](x) works

@ringabout ringabout mentioned this pull request Jan 2, 2021
@timotheecour
Copy link
Member

timotheecour commented Jan 6, 2021

@xflywind

It's not easy to support JS backend.

we can probably do something similar to toBitsImpl (#16592) for js backend, either returning a BigInt or returning array[2, uint32]

maybe this can be unified with a custom type:

when defined js:
  type Bits = array[2, uint32]
else:
  type Bits = uint

proc toBits(x: float): Bits =
  # now we can support c,vm,js

EDIT: see also asBigInt and BigUint64Array

  proc asBigInt(x: float): int64 =
    # result is a `BigInt` type in js, but we cheat the type system
    # and say it is a `int64` type.
    # TODO refactor it using bigInt once jsBigInt is ready, pending pr #1640
    asm """
    const buffer = new ArrayBuffer(8);
    const floatBuffer = new Float64Array(buffer);
    const uintBuffer = new BigUint64Array(buffer);
    floatBuffer[0] = `x`;
    `result` = uintBuffer[0];"""

(with safari support caveat for BigUint64Array timotheecour#509)

@timotheecour timotheecour mentioned this pull request Jan 10, 2021
1 task
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stale Staled PR/issues; remove the label after fixing them
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants