Issue 204069: code review 204069: math: add functions Log2, Nextafter, Fdim, Fmax, Fmin

Issue 204069: code review 204069: math: add functions Log2, Nextafter, Fdim, Fmax, Fmin (Closed)

Can't Edit
Can't Publish+Mail
Start Review

Created:
15 years, 2 months ago by Charlie Dorian

Modified:
15 years, 2 months ago

Reviewers:

CC:
rsc, golang-dev

Visibility:
Public.

Description

math: add functions Log2, Nextafter, Fdim, Fmax, Fmin Add functions, tests and benchmarks. Fix typos in comments in expm1 and hypot_386. Fix Acosh domain error in benchmark test.

Patch Set 2 : code review 204069: math: add functions Log2, Nextafter, Fdim, Fmax, Fmin, Fma #

Total comments: 5

Patch Set 3 : code review 204069: math: add functions Log2, Nextafter, Fdim, Fmax, Fmin, Fma #

Patch Set 4 : code review 204069: math: add functions Log2, Nextafter, Fdim, Fmax, Fmin #

Created: 15 years, 2 months ago

Download [raw] [tar.bz2]

	Unified diffs	Side-by-side diffs	Delta from patch set	Stats (+206 lines, -4 lines)			Patch
M	src/pkg/math/Makefile	View	1 2	1 chunk	+2 lines, -0 lines	0 comments	Download
M	src/pkg/math/all_test.go	View	1 2 3	11 chunks	+132 lines, -1 line	0 comments	Download
M	src/pkg/math/expm1.go	View		1 chunk	+2 lines, -2 lines	0 comments	Download
A	src/pkg/math/fdim.go	View		1 chunk	+29 lines, -0 lines	0 comments	Download
M	src/pkg/math/hypot_386.s	View		1 chunk	+1 line, -1 line	0 comments	Download
M	src/pkg/math/log.go	View		1 chunk	+4 lines, -0 lines	0 comments	Download
M	src/pkg/math/log_386.s	View	1 2 3	1 chunk	+8 lines, -0 lines	0 comments	Download
M	src/pkg/math/log_decl.go	View		1 chunk	+1 line, -0 lines	0 comments	Download
A	src/pkg/math/nextafter.go	View	1 2	1 chunk	+27 lines, -0 lines	0 comments	Download

Messages

Total messages: 7

Expand All Messages | Collapse All Messages

Charlie Dorian

Hello rsc (cc: golang-dev@googlegroups.com), I'd like you to review this change.

15 years, 2 months ago (2010-02-07 20:01:43 UTC) #1

rsc

http://codereview.appspot.com/204069/diff/1001/33 File src/pkg/math/fdim.go (right): http://codereview.appspot.com/204069/diff/1001/33#newcode7 src/pkg/math/fdim.go:7: // Fdim returns the maximum of x-y or 0. ...

15 years, 2 months ago (2010-02-08 05:07:30 UTC) #2

http://codereview.appspot.com/204069/diff/1001/33
File src/pkg/math/fdim.go (right):

http://codereview.appspot.com/204069/diff/1001/33#newcode7
src/pkg/math/fdim.go:7: // Fdim returns the maximum of x-y or 0.
I think these have borderline utility, but they're at least venerable.

http://codereview.appspot.com/204069/diff/1001/34
File src/pkg/math/fdim_386.s (right):

http://codereview.appspot.com/204069/diff/1001/34#newcode6
src/pkg/math/fdim_386.s:6: TEXT ·Fdim(SB),7,$0
There's no need to write these in assembly.
Assembly is for getting at special hardware instructions
that the compiler can't use.  If you look at the output
of 8g -S, the code is almost identical to this once you
unwind the jumps (and 8l will do that):

0000 (x.go:8) TEXT    Fdim+0(SB),$0-24
0005 (x.go:5) FMOVD   y+8(FP),F0
0006 (x.go:5) FMOVD   x+0(FP),F0
0007 (x.go:5) FUCOMPP F0,F1
0008 (x.go:5) FSTSW   ,AX
0009 (x.go:5) SAHF    ,
0010 (x.go:5) JCC     ,17
0012 (x.go:6) FMOVD   x+0(FP),F0
0013 (x.go:6) FSUBD   y+8(FP),F0
0014 (x.go:6) FMOVDP  F0,.noname+16(FP)
0015 (x.go:6) RET     ,
0017 (x.go:8) FMOVD   $(0.00000000000000000e+00),F0
0018 (x.go:8) FMOVDP  F0,.noname+16(FP)
0019 (x.go:8) RET     ,

I'd rather put the effort into making the compiler
use FUCOMI and FLDZ and maybe registerize a little
better than maintain an almost identical copy of
the assembly.

0022 (x.go:16) TEXT    Fmax+0(SB),$0-24
0027 (x.go:13) FMOVD   y+8(FP),F0
0028 (x.go:13) FMOVD   x+0(FP),F0
0029 (x.go:13) FUCOMPP F0,F1
0030 (x.go:13) FSTSW   ,AX
0031 (x.go:13) SAHF    ,
0032 (x.go:13) JCC     ,38
0034 (x.go:14) FMOVD   x+0(FP),F0
0035 (x.go:14) FMOVDP  F0,.noname+16(FP)
0036 (x.go:14) RET     ,
0038 (x.go:16) FMOVD   y+8(FP),F0
0039 (x.go:16) FMOVDP  F0,.noname+16(FP)
0040 (x.go:16) RET     ,

0043 (x.go:24) TEXT    Fmin+0(SB),$0-24
0048 (x.go:21) FMOVD   x+0(FP),F0
0049 (x.go:21) FMOVD   y+8(FP),F0
0050 (x.go:21) FUCOMPP F0,F1
0051 (x.go:21) FSTSW   ,AX
0052 (x.go:21) SAHF    ,
0053 (x.go:21) JCC     ,59
0055 (x.go:22) FMOVD   x+0(FP),F0
0056 (x.go:22) FMOVDP  F0,.noname+16(FP)
0057 (x.go:22) RET     ,
0059 (x.go:24) FMOVD   y+8(FP),F0
0060 (x.go:24) FMOVDP  F0,.noname+16(FP)
0061 (x.go:24) RET     ,

http://codereview.appspot.com/204069/diff/1001/36
File src/pkg/math/fma.go (right):

http://codereview.appspot.com/204069/diff/1001/36#newcode7
src/pkg/math/fma.go:7: // Fma returns x*y + z
I don't think this is worth having.
It's trying to get at a particular machine instruction,
at the cost of a function call.  It will be more efficient if
we just make the compiler use the instruction where 
appropriate.

http://codereview.appspot.com/204069/diff/1001/37
File src/pkg/math/fma_386.s (right):

http://codereview.appspot.com/204069/diff/1001/37#newcode6
src/pkg/math/fma_386.s:6: TEXT ·Fma(SB),7,$0
I think the compiler beat you on this one.  ;-)

0000 (xx.go:3) TEXT    Fma+0(SB),$0-32
0001 (xx.go:3) FMOVD   x+0(FP),F0
0002 (xx.go:3) FMULD   y+8(FP),F0
0003 (xx.go:3) FADDD   z+16(FP),F0
0004 (xx.go:3) FMOVDP  F0,.noname+24(FP)
0005 (xx.go:3) RET     ,

http://codereview.appspot.com/204069/diff/1001/43
File src/pkg/math/nextafter.go (right):

http://codereview.appspot.com/204069/diff/1001/43#newcode21
src/pkg/math/nextafter.go:21: case x > 0:
case y > x == x > 0:
    r = Float64frombits(Float64bits(x) + 1)
default:
    r = Float64frombits(Float64bits(x) - 1)

Charlie Dorian

Hello rsc (cc: golang-dev@googlegroups.com), Please take another look.

15 years, 2 months ago (2010-02-08 05:39:43 UTC) #3

Charlie Dorian

I've removed the Fma function and assembly versions of Fdim, Fmax and Fmin. My tests ...

15 years, 2 months ago (2010-02-08 05:45:51 UTC) #4

I've removed the Fma function and assembly versions of Fdim, Fmax and Fmin.
My tests had shown the 386 versions to be about twice as fast, but it may
just be where the code is in memory.  (You can see that on the benchmarks
page of the Go dashboard.)

On Mon, Feb 8, 2010 at 12:07 AM, <rsc@golang.org> wrote:

>
> http://codereview.appspot.com/204069/diff/1001/33
> File src/pkg/math/fdim.go (right):
>
> http://codereview.appspot.com/204069/diff/1001/33#newcode7
> src/pkg/math/fdim.go:7: // Fdim returns the maximum of x-y or 0.
> I think these have borderline utility, but they're at least venerable.
>
> http://codereview.appspot.com/204069/diff/1001/34
> File src/pkg/math/fdim_386.s (right):
>
> http://codereview.appspot.com/204069/diff/1001/34#newcode6
> src/pkg/math/fdim_386.s:6: TEXT ·Fdim(SB),7,$0
> There's no need to write these in assembly.
> Assembly is for getting at special hardware instructions
> that the compiler can't use.  If you look at the output
> of 8g -S, the code is almost identical to this once you
> unwind the jumps (and 8l will do that):
>
> 0000 (x.go:8) TEXT    Fdim+0(SB),$0-24
> 0005 (x.go:5) FMOVD   y+8(FP),F0
> 0006 (x.go:5) FMOVD   x+0(FP),F0
> 0007 (x.go:5) FUCOMPP F0,F1
> 0008 (x.go:5) FSTSW   ,AX
> 0009 (x.go:5) SAHF    ,
> 0010 (x.go:5) JCC     ,17
> 0012 (x.go:6) FMOVD   x+0(FP),F0
> 0013 (x.go:6) FSUBD   y+8(FP),F0
> 0014 (x.go:6) FMOVDP  F0,.noname+16(FP)
> 0015 (x.go:6) RET     ,
> 0017 (x.go:8) FMOVD   $(0.00000000000000000e+00),F0
> 0018 (x.go:8) FMOVDP  F0,.noname+16(FP)
> 0019 (x.go:8) RET     ,
>
> I'd rather put the effort into making the compiler
> use FUCOMI and FLDZ and maybe registerize a little
> better than maintain an almost identical copy of
> the assembly.
>
> 0022 (x.go:16) TEXT    Fmax+0(SB),$0-24
> 0027 (x.go:13) FMOVD   y+8(FP),F0
> 0028 (x.go:13) FMOVD   x+0(FP),F0
> 0029 (x.go:13) FUCOMPP F0,F1
> 0030 (x.go:13) FSTSW   ,AX
> 0031 (x.go:13) SAHF    ,
> 0032 (x.go:13) JCC     ,38
> 0034 (x.go:14) FMOVD   x+0(FP),F0
> 0035 (x.go:14) FMOVDP  F0,.noname+16(FP)
> 0036 (x.go:14) RET     ,
> 0038 (x.go:16) FMOVD   y+8(FP),F0
> 0039 (x.go:16) FMOVDP  F0,.noname+16(FP)
> 0040 (x.go:16) RET     ,
>
> 0043 (x.go:24) TEXT    Fmin+0(SB),$0-24
> 0048 (x.go:21) FMOVD   x+0(FP),F0
> 0049 (x.go:21) FMOVD   y+8(FP),F0
> 0050 (x.go:21) FUCOMPP F0,F1
> 0051 (x.go:21) FSTSW   ,AX
> 0052 (x.go:21) SAHF    ,
> 0053 (x.go:21) JCC     ,59
> 0055 (x.go:22) FMOVD   x+0(FP),F0
> 0056 (x.go:22) FMOVDP  F0,.noname+16(FP)
> 0057 (x.go:22) RET     ,
> 0059 (x.go:24) FMOVD   y+8(FP),F0
> 0060 (x.go:24) FMOVDP  F0,.noname+16(FP)
> 0061 (x.go:24) RET     ,
>
> http://codereview.appspot.com/204069/diff/1001/36
> File src/pkg/math/fma.go (right):
>
> http://codereview.appspot.com/204069/diff/1001/36#newcode7
> src/pkg/math/fma.go:7: // Fma returns x*y + z
> I don't think this is worth having.
> It's trying to get at a particular machine instruction,
> at the cost of a function call.  It will be more efficient if
> we just make the compiler use the instruction where
> appropriate.
>
> http://codereview.appspot.com/204069/diff/1001/37
> File src/pkg/math/fma_386.s (right):
>
> http://codereview.appspot.com/204069/diff/1001/37#newcode6
> src/pkg/math/fma_386.s:6: TEXT ·Fma(SB),7,$0
> I think the compiler beat you on this one.  ;-)
>
> 0000 (xx.go:3) TEXT    Fma+0(SB),$0-32
> 0001 (xx.go:3) FMOVD   x+0(FP),F0
> 0002 (xx.go:3) FMULD   y+8(FP),F0
> 0003 (xx.go:3) FADDD   z+16(FP),F0
> 0004 (xx.go:3) FMOVDP  F0,.noname+24(FP)
> 0005 (xx.go:3) RET     ,
>
> http://codereview.appspot.com/204069/diff/1001/43
> File src/pkg/math/nextafter.go (right):
>
> http://codereview.appspot.com/204069/diff/1001/43#newcode21
> src/pkg/math/nextafter.go:21: case x > 0:
> case y > x == x > 0:
>    r = Float64frombits(Float64bits(x) + 1)
> default:
>    r = Float64frombits(Float64bits(x) - 1)

Done.  Very nice way to combine those cases!

>
>
> http://codereview.appspot.com/204069/show
>

Charlie Dorian

Hello rsc (cc: golang-dev@googlegroups.com), Please take another look.

15 years, 2 months ago (2010-02-09 16:09:02 UTC) #5

rsc

15 years, 2 months ago (2010-02-09 21:33:15 UTC) #7

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

math: add functions Log2, Nextafter, Fdim, Fmax, Fmin

Add functions, tests and benchmarks. Fix typos in comments
in expm1 and hypot_386.  Fix Acosh domain error in benchmark
test.

R=rsc
CC=golang-dev
http://codereview.appspot.com/204069

Committer: Russ Cox <rsc@golang.org>

Expand All Messages | Collapse All Messages