code review 6210071: sync/atomic: use cas64 to implement {Load,Store,Add}{Ui...

sync/atomic: use cas64 to implement {Load,Store,Add}{Uint,Int}64 on Linux/ARM
        Now with GOARM=5 our all.bash should pass on ARMv5 systems.
        Fixes issue 3331.

[minux1, 2012-05-18 20:46:25]

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

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

[minux1, 2012-05-18 20:54:28]

The code might seem to be doing a lot of needless load/store.

But the truth is, our calling convention dictate that nearly all register could
be clobbered after a function call and all arguments should be passed on
stack. And each these functions must call cas64. What's worse, one code
patch in cas64 involves calling another C function, so if don't split and 
provide 3 implementations for each of these 3 functions (like we did for
cas64), we can't do much optimization to them (except use LDRD/STRD
to replace the register pair load/store), IMHO.

Of course, if you have any optimization suggestion, I'm eager to learn.

Also note that, to make all.bash pass on ARMv5 systems, you must enlarge
the timeout values in run.bash (warning, it will take a long time to finish).

[rsc, 2012-05-18 21:03:17]

If they're just calling CAS, it seems like you could write them in Go
instead of assembly?

Russ

[minux1, 2012-05-18 21:50:24]

PTAL.

On Sat, May 19, 2012 at 5:02 AM, Russ Cox <rsc@golang.org> wrote:

> If they're just calling CAS, it seems like you could write them in Go
> instead of assembly?

it seems I have to introduce one level of indirection to keep all function
declaration
together at doc.go?

[dvyukov, 2012-05-19 10:32:36]

Just out of curiosity

http://codereview.appspot.com/6210071/diff/9002/src/pkg/sync/atomic/doc.go
File src/pkg/sync/atomic/doc.go (right):

http://codereview.appspot.com/6210071/diff/9002/src/pkg/sync/atomic/doc.go#ne...
src/pkg/sync/atomic/doc.go:61: return addUint64(val, delta)
Is it inlined?

[minux1, 2012-05-19 10:41:54]

On 2012/05/19 10:32:36, dvyukov wrote:
http://codereview.appspot.com/6210071/diff/9002/src/pkg/sync/atomic/doc.go#ne...
> src/pkg/sync/atomic/doc.go:61: return addUint64(val, delta)
> Is it inlined?
it won't.
IIUC, gc can only inline functions compiled by itself, so only linux/arm version
is eligible to inlining, but it won't either, because addUint64 isn't a leaf
function.

I'm afraid, this change is going to slow every port down, if we keep these
declarations
in one place in doc.go. sync/atomic is performance critical code, so I think we
should
allow some documentation duplication and move related functions out of doc.go

what do you think?

[dvyukov, 2012-05-19 11:00:33]

On 2012/05/19 10:41:54, minux wrote:
> On 2012/05/19 10:32:36, dvyukov wrote:
>
http://codereview.appspot.com/6210071/diff/9002/src/pkg/sync/atomic/doc.go#ne...
> > src/pkg/sync/atomic/doc.go:61: return addUint64(val, delta)
> > Is it inlined?
> it won't.
> IIUC, gc can only inline functions compiled by itself, so only linux/arm
version
> is eligible to inlining, but it won't either, because addUint64 isn't a leaf
> function.

I see.

> I'm afraid, this change is going to slow every port down, if we keep these
> declarations
> in one place in doc.go. sync/atomic is performance critical code, so I think
we
> should
> allow some documentation duplication and move related functions out of doc.go
> 
> what do you think?

I think it's nice to keep these function fast, because performance is usually
the only reason for using them.
FWIW, I do not mind if several functions and doc comments are duplicated. But I
would wait for Russ answer before reimplementing it.

[rsc, 2012-05-21 17:39:08]

Hi

I'd rather leave the existing assembly alone, and then in the ARM case where you
want to use Go code, have the assembly do a jump to the Go implementation.  This
is the style we've used in other places, such as package big, and it seems to
work well.

Thanks.

[minux1, 2012-05-21 18:20:22]

PTAL.

Also exchanged the code for CompareAndSwapUint64 and
CompareAndSwapInt64, because we usually implement the
unsigned version, make the signed version jump to it.

[rsc, 2012-05-22 03:11:11]

LGTM

On Mon, May 21, 2012 at 2:20 PM,  <minux.ma@gmail.com> wrote:
> we usually implement the
> unsigned version, make the signed version jump to it.

The signed and unsigned version are identical
except for their type signatures, no?
Either way, it's fine.

[minux1, 2012-05-22 18:02:09]

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

sync/atomic: use cas64 to implement {Load,Store,Add}{Uint,Int}64 on Linux/ARM
        Now with GOARM=5 our all.bash should pass on ARMv5 systems.
        Fixes issue 3331.

R=golang-dev, rsc, dvyukov
CC=golang-dev
http://codereview.appspot.com/6210071

[minux1, 2012-05-22 18:03:51]

On 2012/05/22 03:11:11, rsc wrote:
> On Mon, May 21, 2012 at 2:20 PM,  <mailto:minux.ma@gmail.com> wrote:
> > we usually implement the
> > unsigned version, make the signed version jump to it.
> The signed and unsigned version are identical
> except for their type signatures, no?
Yes. I changed that for consistency with other functions.