code review 126490043: runtime: convert float64toint64, float64touint64 to Go

runtime: convert float64toint64, float64touint64 to Go

This is a very dumb translation to keep the code as close to the original C as possible.

[dave_cheney.net, 2014-08-21 07:05:23]

Hello khr@golang.org, minux@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

I'd like you to review this change to
https://code.google.com/p/go

[khr, 2014-08-21 17:06:22]

https://codereview.appspot.com/126490043/diff/40002/src/pkg/runtime/vlrt_arm.go
File src/pkg/runtime/vlrt_arm.go (right):

https://codereview.appspot.com/126490043/diff/40002/src/pkg/runtime/vlrt_arm....
src/pkg/runtime/vlrt_arm.go:81: yhi = dvl.hi /* causes something awful */
I don't understand this translation.  It used to do a C float->int conversion,
now it just grabs half of the float representation.  They can't be equivalent,
can they?

[dave_cheney.net, 2014-08-22 01:42:21]

Hello khr@golang.org, minux@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[dave_cheney.net, 2014-08-22 01:43:52]

https://codereview.appspot.com/126490043/diff/40002/src/pkg/runtime/vlrt_arm.go
File src/pkg/runtime/vlrt_arm.go (right):

https://codereview.appspot.com/126490043/diff/40002/src/pkg/runtime/vlrt_arm....
src/pkg/runtime/vlrt_arm.go:81: yhi = dvl.hi /* causes something awful */
On 2014/08/21 17:06:22, khr wrote:
> I don't understand this translation.  It used to do a C float->int conversion,
> now it just grabs half of the float representation.  They can't be equivalent,
> can they?

You are correct, this was wrong. I was worried that float64 -> uint32 would
cause recursion into this or some other helper, but it looks like I was wrong.

PTAL.

[minux, 2014-08-22 02:19:01]

I'd suggest we consolidate all this into vlrt.go
rather than vlrt_arm.go.

the {uint，float}64to{float,uint}64 functions are only
used by 5g, but it won't do any harm to always have
it. The linker will do the right job.

e.g. if/when we add support for FPU-less MIPS32,
we will need to rename the file and add build tags,
because the name is already properly namespaced,
it makes sense to put into a generic vlrt.go, and
if we care about compilation speed (which I highly
doubt we will), we can apply build tags as needed.

[dave_cheney.net, 2014-08-22 02:21:21]

Yup. I can do that, it is obvious in retrospect.

Wrt, the translation of _d2v, does that look sane now?
On 22 Aug 2014 12:19, <minux@golang.org> wrote:

> I'd suggest we consolidate all this into vlrt.go
> rather than vlrt_arm.go.
>
> the {uint，float}64to{float,uint}64 functions are only
> used by 5g, but it won't do any harm to always have
> it. The linker will do the right job.
>
> e.g. if/when we add support for FPU-less MIPS32,
> we will need to rename the file and add build tags,
> because the name is already properly namespaced,
> it makes sense to put into a generic vlrt.go, and
> if we care about compilation speed (which I highly
> doubt we will), we can apply build tags as needed.
>
> https://codereview.appspot.com/126490043/
>

[minux, 2014-08-22 02:39:48]

https://codereview.appspot.com/126490043/diff/70001/src/pkg/runtime/vlrt_arm.go
File src/pkg/runtime/vlrt_arm.go (right):

https://codereview.appspot.com/126490043/diff/70001/src/pkg/runtime/vlrt_arm....
src/pkg/runtime/vlrt_arm.go:31: lo, hi uint32
the definition is not big-endian-safe,
could we use a uint64 variable and load
the float64 as a whole into uint64 and
then separate into lo and hi as usual?

(I know the original C version isn't
either, but I think we have higher standard
for Go code.)

[dave_cheney.net, 2014-08-22 05:17:56]

PTAL. 

vlrt_arm.go renamed to vlrt.go with a build tag, as this file is not based on
the inferno code I have not hg copied it.

Minux, I agree _d2v is not endian safe. 

As Josh is also working in this area I'd like to land what is in this proposal
now to avoid blocking him, then tackle the TODO as a followup CL along with
other cleanups to make the function more idiomatic.

[rsc, 2014-08-22 14:47:59]

In the Go compilers (in contrast to the C compilers) I believe we always emit
machine code for 64-bit bit operations (shift, add, etc). So the type vlong
struct should not be necessary. 

The problem in the C compilers was that all the 64-bit math, even shift and mask
and such, was calling library routines, so the library routines themselves could
not make use of 64-bit numbers.

In this case I think you can replace all the references to vlong with the real
uint64.

[dave_cheney.net, 2014-08-23 06:02:30]

Hello khr@golang.org, minux@golang.org, rsc@golang.org, josharian@gmail.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[rsc, 2014-08-23 21:14:42]

LGTM

This code is awful and can be cleaned up now that we have 64-bit shifts, but
it's not worth doing now and possibly introducing bugs. Please do fix the hg
history/copyright issue before submitting.

https://codereview.appspot.com/126490043/diff/150001/src/pkg/runtime/vlrt.go
File src/pkg/runtime/vlrt.go (right):

https://codereview.appspot.com/126490043/diff/150001/src/pkg/runtime/vlrt.go#...
src/pkg/runtime/vlrt.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
This is a direct copy of the code in vlrt_arm.c. Use hg cp -A to tell hg that,
and copy the copyright notice from that .c file.

[dave_cheney.net, 2014-08-23 23:26:07]

Thanks Russ. 

I've restored the original copyright header, I'll submit this shortly.

https://codereview.appspot.com/126490043/diff/150001/src/pkg/runtime/vlrt.go
File src/pkg/runtime/vlrt.go (right):

https://codereview.appspot.com/126490043/diff/150001/src/pkg/runtime/vlrt.go#...
src/pkg/runtime/vlrt.go:1: // Copyright 2014 The Go Authors. All rights
reserved.
On 2014/08/23 21:14:42, rsc wrote:
> This is a direct copy of the code in vlrt_arm.c. Use hg cp -A to tell hg that,
> and copy the copyright notice from that .c file.

Done

[dave_cheney.net, 2014-08-23 23:38:51]

*** Submitted as https://code.google.com/p/go/source/detail?r=a12bd7115455 ***

runtime: convert float64toint64, float64touint64 to Go

This is a very dumb translation to keep the code as close to the original C as
possible.

LGTM=rsc
R=khr, minux, rsc, josharian
CC=golang-codereviews
https://codereview.appspot.com/126490043