code review 4551082: math: add sqrt_arm.s and sqrtGoC.go as fallback to soft...

math: add sqrt_arm.s and sqrtGoC.go as fallback to soft fp emulation
5a: add SQRTF and SQRTD
5l: add ASQRTF and ASQRTD

Use ARMv7 VFP VSQRT instruction to speed up math.Sqrt

[fango, 2011-06-01 14:02:01]

Resent since CL not appeared on golang-dev

[rsc, 2011-06-02 14:38:27]

Looks fine to me but will break ARM5.  You'll need to have
the software floating point simulator in $GOROOT/src/pkg/runtime/arm/softfloat.c
call math·sqrtGoC to implement that instruction, where sqrtGoC is defined

package math
func sqrtGoC(f float64, r *float64) {
    *r = sqrtGo(f)
}

and has C prototype

void math·sqrtGoC(float64, *float64);

and then you'll have to change 5l to 
automatically load math.a the same way
that it does runtime.a.

http://codereview.appspot.com/4551082/diff/2001/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

http://codereview.appspot.com/4551082/diff/2001/src/cmd/5l/asm.c#newcode1690
src/cmd/5l/asm.c:1690: (0xc<<4);
merge with previous line

http://codereview.appspot.com/4551082/diff/2001/src/cmd/5l/asm.c#newcode1691
src/cmd/5l/asm.c:1691: 
why the blank line?

[fango, 2011-06-03 01:42:10]

> and then you'll have to change 5l to
> automatically load math.a the same way
> that it does runtime.a.

This is new. Supporting both ARM5,ARM6, ARMv7, ARMv7_neon are going to
be messy. Soft fp only applicable to GOARM=5 anyway. So I would like
to add one more env, GOARCH_VER to pkg/math/Makefile:

OFILES=\
        $(OFILES_$(GOARCH)$(GOARCH_VER))

So we can have below rule only build when GOARCH_VER=v7
OFILES_armv7=\
        fabs_arm.$O\
        sqrt_arm.$O\

The neon support for math sin/cos etc., then can use GOARCH_VER=v7_neon.


> http://codereview.appspot.com/4551082/

Please ignore this. I'm new to CL, not aware the 'hg mail' at the
first time. Please take another look at
http://codereview.appspot.com/4548085/, Sorry for the confusion.

fango

[rsc, 2011-06-03 01:52:58]

> This is new. Supporting both ARM5,ARM6, ARMv7, ARMv7_neon are going to
> be messy. Soft fp only applicable to GOARM=5 anyway. So I would like
> to add one more env, GOARCH_VER to pkg/math/Makefile:

I don't want to add a new variable.  Right now you can compile
everything with GOARCH=arm and you only need the GOARM
value during the call to 5l.  The libraries are all the same and
don't need to be built separately for different ARMs.  I don't mind
if the software floating point generates calls to the math library
though.

There are really only two ARMs we are supporting:
old, using software floating point (we call it GOARM=5)
and new, using VFP floating point (we call it not setting GOARM).
All the fine gradations in the middle I am happy to ignore.

Russ

[fango, 2011-06-03 06:22:49]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[fango, 2011-06-03 06:30:44]

Hello rsc@golang.org (cc: golang-dev@googlegroups.com),

Please take another look.

[fango, 2011-06-03 06:36:10]

On 2011/06/03 06:30:44, Fango wrote:
> Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com),
> 
> Please take another look.

So sorry. hg change 4551082 did not add all files I changed. Have to manually
track and add them. 

I don't have a ARM5 board, so just GOARM=5 to output arm5, and measured a big
diff in timing. 

time ./5.out
PASS

real    0m0.047s
user    0m0.023s
sys     0m0.016s


time ./arm5
PASS

real    0m0.395s
user    0m0.391s
sys     0m0.000s

[dave_cheney.net, 2011-06-03 06:46:15]

Hi Howard,

You can use

hg file 4551082 file1 file2 file3

to add files to an exist CL

Cheers

Dave

On Fri, Jun 3, 2011 at 4:36 PM,  <fan.howard@gmail.com> wrote:
> On 2011/06/03 06:30:44, Fango wrote:
>>
>> Hello mailto:rsc@golang.org (cc: mailto:golang-dev@googlegroups.com),
>
>> Please take another look.
>
> So sorry. hg change 4551082 did not add all files I changed. Have to
> manually track and add them.
>
> I don't have a ARM5 board, so just GOARM=5 to output arm5, and measured
> a big diff in timing.
>
> time ./5.out
> PASS
>
> real    0m0.047s
> user    0m0.023s
> sys     0m0.016s
>
>
> time ./arm5
> PASS
>
> real    0m0.395s
> user    0m0.391s
> sys     0m0.000s
>
>
> http://codereview.appspot.com/4551082/
>

[captn, 2011-06-03 08:20:28]

On 2011/06/03 01:52:58, rsc wrote:
> > This is new. Supporting both ARM5,ARM6, ARMv7, ARMv7_neon are going to
> > be messy. Soft fp only applicable to GOARM=5 anyway. So I would like
> > to add one more env, GOARCH_VER to pkg/math/Makefile:
> 
> I don't want to add a new variable.  Right now you can compile
> everything with GOARCH=arm and you only need the GOARM
> value during the call to 5l.  The libraries are all the same and
> don't need to be built separately for different ARMs.  I don't mind
> if the software floating point generates calls to the math library
> though.
> 
> There are really only two ARMs we are supporting:
> old, using software floating point (we call it GOARM=5)
> and new, using VFP floating point (we call it not setting GOARM).
> All the fine gradations in the middle I am happy to ignore.

Yes, but it's not only old/new. Sometimes is the FPU powered down
to save the battery or the SoC manufacturer doesn't even bother
to put the FPU core on the chip.

The android NDK similarly targets the user-space for
"soft" FP (armv5te) and "hard" FP (armv7-a).
So without the FPU there would be in Go always an opt-in for
GOARM=5 and that's fine (at least for me).

For some reason I can't get run even a GOARM=5 binary on an ARMv6
without a FPU. This should be supported, no?

Martin

[fango, 2011-06-03 08:51:36]

> All the fine gradations in the middle I am happy to ignore.

If I guessed right, Russ would like Go to support only ARM5 and ARMv7+Neon, not
anything in between. Those quirk ARMv7 (Tegra 2 etc.,) would hopefully fade out
quickly, like 286.

[rsc, 2011-06-03 15:19:45]

> For some reason I can't get run even a GOARM=5 binary on an ARMv6
> without a FPU. This should be supported, no?

Running a GOARM=5 binary on an ARMv6 should work.

Russ

[dave_cheney.net, 2011-06-06 06:28:09]

Looks good on arm5

[rsc, 2011-06-07 19:10:54]

The new code looks fine but if you view the diffs,
you've undone a bunch of other changes that I made.
Could you please read through the diffs and revert
the ones that are not part of your sqrt change?

Thanks.
Russ

[fango, 2011-06-08 00:18:58]

Hello rsc@golang.org, dave@cheney.net, m@capitanio.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[fango, 2011-06-08 00:25:13]

Sorry I don't quite get what you mean. So I did a  'hg sync' and merge, then `hg
mail 4551082` to confirm that is your intention. But that causes a comping
error:

/home/go/go/src/cmd/5l/asm.c:381:4: error: implicit declaration of function
'asmelfsym32'

[fango, 2011-06-08 01:06:15]

Hello rsc@golang.org, dave@cheney.net, m@capitanio.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[fango, 2011-06-08 01:14:40]

Hello rsc@golang.org, dave@cheney.net, m@capitanio.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[fango, 2011-06-08 01:15:33]

Hi Russ,

I did a `hg update default

[fango, 2011-06-08 01:18:17]

Sorry, switching from vim did byte me. Anyway, I did a hg update default and
manual patched the changes I made on SQRTF/D. Please take another look. Thanks.

[rsc, 2011-06-08 02:47:08]

looks great; just the file nit below.
thanks!

http://codereview.appspot.com/4551082/diff/27001/src/pkg/math/Makefile
File src/pkg/math/Makefile (right):

http://codereview.appspot.com/4551082/diff/27001/src/pkg/math/Makefile#newcode91
src/pkg/math/Makefile:91: sqrtGoC.go\
please drop this code in sqrt_port.go instead.
it's too small to be worth another file.

[fango, 2011-06-08 02:54:13]

Hello rsc@golang.org, dave@cheney.net, m@capitanio.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[rsc, 2011-06-08 03:05:46]

LGTM

Thanks!

[fango, 2011-06-08 04:40:09]

Hello rsc@golang.org, dave@cheney.net, m@capitanio.org (cc:
golang-dev@googlegroups.com),

Please take another look.

[fango, 2011-06-08 04:42:27]

Patch 9 fixed a typo in Makefile, and gofmt -w sqrt_port.go.

[rsc, 2011-06-08 04:45:20]

will submit tomorrow once i sort some other arm stuff

[rsc, 2011-06-09 21:19:11]

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

math: add sqrt_arm.s and sqrtGoC.go as fallback to soft fp emulation
5a: add SQRTF and SQRTD
5l: add ASQRTF and ASQRTD

Use ARMv7 VFP VSQRT instruction to speed up math.Sqrt

R=rsc, dave, m
CC=golang-dev
http://codereview.appspot.com/4551082

Committer: Russ Cox <rsc@golang.org>