Hello golang-dev@googlegroups.com, I'd like you to review this change to https://go.googlecode.com/hg/
LGTM http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_386.h File src/pkg/runtime/arch_386.h (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_386.h#ne... src/pkg/runtime/arch_386.h:8: void runtime·prefetch(const volatile void*); const and volatile have nearly no effect in our compiler and are conventionally not mentioned in our header. Please remove them. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_amd64.h File src/pkg/runtime/arch_amd64.h (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_amd64.h#... src/pkg/runtime/arch_amd64.h:8: void runtime·prefetch(const volatile void*); Same. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/asm_386.s File src/pkg/runtime/asm_386.s (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/asm_386.s#new... src/pkg/runtime/asm_386.s:326: XORL AX, AX Please use MOVL $0, AX like in the surrounding code. It is the linker's job to play tricks like these, not the programmer's. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/asm_amd64.s File src/pkg/runtime/asm_amd64.s (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/asm_amd64.s#n... src/pkg/runtime/asm_amd64.s:368: XORL AX, AX Same. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_386.c File src/pkg/runtime/atomic_386.c (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_386.c#... src/pkg/runtime/atomic_386.c:28: while(!runtime·cas64(addr, &old, old+v)); Doesn't this generate a warning when you build package runtime? Either way, please make it multiple lines: while(!runtime.cas64(addr, &old, old+v)) { // nothing } or for(;;) { if(runtime.cas64(addr, &old, old+v)) break; } It's too easy to misread as written. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_arm.c File src/pkg/runtime/atomic_arm.c (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_arm.c#... src/pkg/runtime/atomic_arm.c:13: #define LOCK(addr) (&locktab[((uintptr)addr>>3)%nelem(locktab)].l) (uintptr)(addr)
Hello golang-dev@googlegroups.com, rsc@golang.org (cc: golang-dev@googlegroups.com), Please take another look.
http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_386.h File src/pkg/runtime/arch_386.h (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_386.h#ne... src/pkg/runtime/arch_386.h:8: void runtime·prefetch(const volatile void*); On 2012/04/05 14:24:54, rsc wrote: > const and volatile have nearly no effect in our compiler and are conventionally > not mentioned in our header. Please remove them. Done. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_amd64.h File src/pkg/runtime/arch_amd64.h (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/arch_amd64.h#... src/pkg/runtime/arch_amd64.h:8: void runtime·prefetch(const volatile void*); On 2012/04/05 14:24:54, rsc wrote: > Same. Done. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/asm_386.s File src/pkg/runtime/asm_386.s (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/asm_386.s#new... src/pkg/runtime/asm_386.s:326: XORL AX, AX On 2012/04/05 14:24:54, rsc wrote: > Please use MOVL $0, AX like in the surrounding code. > It is the linker's job to play tricks like these, not the > programmer's. Done. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_386.c File src/pkg/runtime/atomic_386.c (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_386.c#... src/pkg/runtime/atomic_386.c:28: while(!runtime·cas64(addr, &old, old+v)); On 2012/04/05 14:24:54, rsc wrote: > Doesn't this generate a warning when you build package runtime? I do not see any warnings. > Either way, please make it multiple lines: > > while(!runtime.cas64(addr, &old, old+v)) { > // nothing > } > Done. > or > > for(;;) { > if(runtime.cas64(addr, &old, old+v)) > break; > } > > It's too easy to misread as written. http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_arm.c File src/pkg/runtime/atomic_arm.c (right): http://codereview.appspot.com/5985047/diff/3011/src/pkg/runtime/atomic_arm.c#... src/pkg/runtime/atomic_arm.c:13: #define LOCK(addr) (&locktab[((uintptr)addr>>3)%nelem(locktab)].l) On 2012/04/05 14:24:54, rsc wrote: > (uintptr)(addr) Ouch! Done
LGTM Maybe we only issue the warning for if(); and not while();. Either way, thanks for changing it to be harder to misread. http://codereview.appspot.com/5985047/diff/6012/src/pkg/runtime/asm_386.s File src/pkg/runtime/asm_386.s (right): http://codereview.appspot.com/5985047/diff/6012/src/pkg/runtime/asm_386.s#new... src/pkg/runtime/asm_386.s:326: XORL AX, AX MOVL $0, AX
*** Submitted as http://code.google.com/p/go/source/detail?r=dbf4d5bba619 *** runtime: add 64-bit atomics This is factored out part of: http://codereview.appspot.com/5279048/ (Parallel GC) R=golang-dev, rsc CC=golang-dev http://codereview.appspot.com/5985047
