code review 133790043: runtime: implement 64 bit division in Go

runtime: implement 64 bit division in Go

[josharian, 2014-08-26 20:52:19]

Hello minux@golang.org, rsc@golang.org, remyoudompheng@gmail.com,
dave@cheney.net (cc: golang-codereviews@googlegroups.com),

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

[dave_cheney.net, 2014-08-26 22:24:57]

Drive by comment,

I'd really like to have one vlrt.go file, mainly for artistic reasons, but
minux did note he would find it useful if MIPS is added in the future.

The main difference appears to be dodiv, could you use a constant branch,
if GOARCH="arm" { slowdiv(); return } to integrate the two files ?

Thanks for having a crack at optimising this call for 386, do you have any
benchmarks? If not I can write some up ?
On 27 Aug 2014 06:52, <josharian@gmail.com> wrote:

> Reviewers: minux, rsc, remyoudompheng, dfc,
>
> Message:
> Hello minux@golang.org, rsc@golang.org, remyoudompheng@gmail.com,
> dave@cheney.net (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go
>
>
> Description:
> runtime: implement 64 bit division in Go
>
> Please review this at https://codereview.appspot.com/133790043/
>
> Affected files (+244, -49 lines):
>   M src/pkg/runtime/vlop_386.s
>   M src/pkg/runtime/vlrt.go
>   M src/pkg/runtime/vlrt_386.c
>   A src/pkg/runtime/vlrt_386.go
>   M src/pkg/runtime/vlrt_arm.c
>   A src/pkg/runtime/vlrt_arm.go
>
>
>

[josharian, 2014-08-26 22:45:38]

> The main difference appears to be dodiv, could you use a constant branch,
> if GOARCH="arm" { slowdiv(); return } to integrate the two files ?

Happy to do that, but how should we handle the _mul64by32 and _div64by32
function prototypes? Seems ugly to put in arm stubs that do nothing / panic.


> Thanks for having a crack at optimising this call for 386, do you have any
> benchmarks? If not I can write some up ?

Actually, the main optimizing was for readability and not replacing things like
uint32(n>>32) with nhi. (The shift expression compiles down to just a register
usage.)

I'd *love* some benchmarks. Cf vlop_arm_test.go for some 32 bit benchmarks;
might be a useful starting place.

[rsc, 2014-08-27 01:08:59]

not lgtm

wait for my assembly changes please

[josharian, 2014-08-28 23:37:27]

> wait for my assembly changes please

I'm not sure whether the coast is clear yet. If not, please ignore.

* Updated to tip. No assembly changes now other than prepending runtime·.
* arm/386 delta reduced. I see no way to go further without significant 386
degradation.
* test/64bit.go covers corner cases. I'm running general randomized Go-vs-C
tests using CL 131620043 (vlrt_test.go).
* Real benchmarks are in dfc's court, but initial CPU profiling suggests no
unpleasant surprises lurk.

[dave_cheney.net, 2014-08-29 01:04:22]

On 2014/08/28 23:37:27, josharian wrote:
> > wait for my assembly changes please
> 
> I'm not sure whether the coast is clear yet. If not, please ignore.
> 
> * Updated to tip. No assembly changes now other than prepending runtime·.
> * arm/386 delta reduced. I see no way to go further without significant 386
> degradation.
> * test/64bit.go covers corner cases. I'm running general randomized Go-vs-C
> tests using CL 131620043 (vlrt_test.go).
> * Real benchmarks are in dfc's court, but initial CPU profiling suggests no
> unpleasant surprises lurk.

I think you can move the vlrt_386.go decks into the main file. As they are not
called on arm, put some stubs in vlop_arm.s that load 0 into R0 and jump to it
to crash the process.

[dave_cheney.net, 2014-08-29 01:04:50]

I have some benchmarks in the works. I'll try to get the posted today.

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

https://codereview.appspot.com/133790043/diff/20001/src/pkg/runtime/vlrt.go#n...
src/pkg/runtime/vlrt.go:21: func uint64div(n, d uint64) (q uint64) {
nit: if you used a named return argument, don't also explicitly return it at the
bottom of the function.

[josharian, 2014-08-29 02:07:58]

> I think you can move the vlrt_386.go decks into the main file. As they are not
called on arm, put some stubs in vlop_arm.s

Done. Still waiting for all.bash to complete.

> I have some benchmarks in the works. I'll try to get the posted today.

Awesome. Thanks, Dave.

>
https://codereview.appspot.com/133790043/diff/20001/src/pkg/runtime/vlrt.go#n...
> src/pkg/runtime/vlrt.go:21: func uint64div(n, d uint64) (q uint64) {
> nit: if you used a named return argument, don't also explicitly return it at
the
> bottom of the function.

Done.

[dave_cheney.net, 2014-08-29 02:09:51]

Nice. Some small nits.

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm.s
File src/pkg/runtime/vlop_arm.s (right):

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm...
src/pkg/runtime/vlop_arm.s:301: MOVW	(R0), R1
newline after this

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm...
src/pkg/runtime/vlop_arm.s:301: MOVW	(R0), R1
MOVW (RO), R1 // crash

Just to make it clear this is deliberate.

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm...
src/pkg/runtime/vlop_arm.s:304: MOVW	(R0), R1
same

[josharian, 2014-08-29 02:11:56]

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm.s
File src/pkg/runtime/vlop_arm.s (right):

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm...
src/pkg/runtime/vlop_arm.s:301: MOVW	(R0), R1
On 2014/08/29 02:09:51, dfc wrote:
> MOVW (RO), R1 // crash
> 
> Just to make it clear this is deliberate.

Done.

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm...
src/pkg/runtime/vlop_arm.s:301: MOVW	(R0), R1
On 2014/08/29 02:09:51, dfc wrote:
> newline after this

Done.

https://codereview.appspot.com/133790043/diff/240001/src/pkg/runtime/vlop_arm...
src/pkg/runtime/vlop_arm.s:304: MOVW	(R0), R1
On 2014/08/29 02:09:51, dfc wrote:
> same

Done.

[rsc, 2014-08-29 02:40:03]

LGTM

https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_386.go
File src/pkg/runtime/vlrt_386.go (right):

https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_386...
src/pkg/runtime/vlrt_386.go:1: // Inferno's libkern/vlrt-arm.c
I usually say the opposite, I know, but there is no Inferno code in this file.
Use the standard Go copyright header.

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

https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_arm...
src/pkg/runtime/vlrt_arm.go:1: // Inferno's libkern/vlrt-arm.c
Same.

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

https://codereview.appspot.com/133790043/diff/260001/src/pkg/runtime/vlrt.go#...
src/pkg/runtime/vlrt.go:249: quo := uint64(0)
use q instead of creating a new variable

https://codereview.appspot.com/133790043/diff/260001/src/pkg/runtime/vlrt.go#...
src/pkg/runtime/vlrt.go:250: for i >= 0 {
for ; i >= 0; i-- {

[dave_cheney.net, 2014-08-29 04:08:13]

LGTM. Please don't wait for me to submit benchmarks before landing
this, land it when the 386 build is passing and you're happy.

On Fri, Aug 29, 2014 at 12:40 PM,  <rsc@golang.org> wrote:
> LGTM
>
>
>
>
https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_386.go
> File src/pkg/runtime/vlrt_386.go (right):
>
>
https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_386...
> src/pkg/runtime/vlrt_386.go:1: // Inferno's libkern/vlrt-arm.c
> I usually say the opposite, I know, but there is no Inferno code in this
> file. Use the standard Go copyright header.
>
>
https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_arm.go
> File src/pkg/runtime/vlrt_arm.go (right):
>
>
https://codereview.appspot.com/133790043/diff/180001/src/pkg/runtime/vlrt_arm...
> src/pkg/runtime/vlrt_arm.go:1: // Inferno's libkern/vlrt-arm.c
> Same.
>
> https://codereview.appspot.com/133790043/diff/260001/src/pkg/runtime/vlrt.go
> File src/pkg/runtime/vlrt.go (right):
>
>
https://codereview.appspot.com/133790043/diff/260001/src/pkg/runtime/vlrt.go#...
> src/pkg/runtime/vlrt.go:249: quo := uint64(0)
> use q instead of creating a new variable
>
>
https://codereview.appspot.com/133790043/diff/260001/src/pkg/runtime/vlrt.go#...
> src/pkg/runtime/vlrt.go:250: for i >= 0 {
> for ; i >= 0; i-- {
>
> https://codereview.appspot.com/133790043/

[josharian, 2014-08-29 16:55:38]

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

runtime: implement 64 bit division in Go

LGTM=rsc, dave
R=minux, rsc, remyoudompheng, dave
CC=golang-codereviews
https://codereview.appspot.com/133790043