code review 109050043: all: remove 'extern register M *m' from runtime

all: remove 'extern register M *m' from runtime

The runtime has historically held two dedicated values g (current goroutine)
and m (current thread) in 'extern register' slots (TLS on x86, real registers
backed by TLS on ARM).

This CL removes the extern register m; code now uses g->m.

On ARM, this frees up the register that formerly held m (R9).
This is important for NaCl, because NaCl ARM code cannot use R9 at all.

The Go 1 macrobenchmarks (those with per-op times >= 10 µs) are unaffected:

BenchmarkBinaryTree17              5491374955     5471024381     -0.37%
BenchmarkFannkuch11                4357101311     4275174828     -1.88%
BenchmarkGobDecode                 11029957       11364184       +3.03%
BenchmarkGobEncode                 6852205        6784822        -0.98%
BenchmarkGzip                      650795967      650152275      -0.10%
BenchmarkGunzip                    140962363      141041670      +0.06%
BenchmarkHTTPClientServer          71581          73081          +2.10%
BenchmarkJSONEncode                31928079       31913356       -0.05%
BenchmarkJSONDecode                117470065      113689916      -3.22%
BenchmarkMandelbrot200             6008923        5998712        -0.17%
BenchmarkGoParse                   6310917        6327487        +0.26%
BenchmarkRegexpMatchMedium_1K      114568         114763         +0.17%
BenchmarkRegexpMatchHard_1K        168977         169244         +0.16%
BenchmarkRevcomp                   935294971      914060918      -2.27%
BenchmarkTemplate                  145917123      148186096      +1.55%

Minux previous reported larger variations, but these were caused by
run-to-run noise, not repeatable slowdowns.

Actual code changes by Minux.
I only did the docs and the benchmarking.

[rsc, 2014-06-16 17:23:46]

Hello minux@golang.org (cc: golang-codereviews@googlegroups.com),

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

[josharian, 2014-06-16 19:09:16]

https://codereview.appspot.com/109050043/diff/40001/doc/asm.html
File doc/asm.html (right):

https://codereview.appspot.com/109050043/diff/40001/doc/asm.html#newcode152
doc/asm.html:152: (Exceptions: the <code>g</code> register renaming on ARM.)
s/Exceptions/Exception/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:753: //MOVQ	g_m(BP), BP
Leftover debug code?

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:761: XORQ	R8, R8
Why not `MOVQ $0, 0(SP)` on the next line and omit this instruction? R8 is not
used again.

[iant, 2014-06-16 20:32:36]

https://codereview.appspot.com/109050043/diff/40001/src/cmd/5a/lex.c
File src/cmd/5a/lex.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/cmd/5a/lex.c#newcode203
src/cmd/5a/lex.c:203: "g",		LREG,	10, // avoid unintentionally clobber m/g using
R9/R10
s|m/g|g|
s|R9/||

https://codereview.appspot.com/109050043/diff/40001/src/liblink/obj6.c
File src/liblink/obj6.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/liblink/obj6.c#newcod...
src/liblink/obj6.c:397: if(ctxt->tlsg == nil)
It's not relevant for this CL, but I don't understand why these two lines do not
appear in obj8.c.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:70: // set up m and g "registers"
s/m and g "registers"/g "register"/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:665: JEQ	2(PC) // TODO
Looks like this should be
    JEQ needm
Looks like that would have worked in the original as well.  And you can drop the
MOVL $0, BP.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:750: JEQ	2(PC)
This should be simple
    JEQ needm
and I don't see any need for the "MOVL $0, BP".

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:559: // void setmg(G*); set m and g. for use by needm.
s/m and //

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:668: // save_gm saves the g and m registers into
pthread-provided
s/save_gm/save_g/

There are remaining calls to save_gm in the new code above.  Shouldn't those
change to save_g?

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:686: TEXT runtime·load_g(SB),NOSPLIT,$0
Shouldn't the calls to load_gm above change to load_g?

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
File src/pkg/runtime/cgo/gcc_darwin_386.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_386.c:85: }
You've dropped the line
    tofree[ntofree++] = k;

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
File src/pkg/runtime/cgo/gcc_darwin_amd64.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_amd64.c:25: * at 0x60 on Leopard / Snow Leopard.
So the offsets are
This sentence should be

So the offset is 0x60+8*0x108 == 0x8a0.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_amd64.c:28: * The linker and runtime hard-code
these constant offsets
s/these constants offsets/this constant offset/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_amd64.c:29: * from %gs where we expect to find m
and g.
s/m and //

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c#n...
src/pkg/runtime/panic.c:492: //byte g;
These two commented out lines should be removed.  Perhaps keep a comment like
  // note that global g points to gsignal

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c#n...
src/pkg/runtime/panic.c:501: if(g->m->locks-g->m->softfloat != 0 ||
g->m->mallocing != 0 || g->m->throwing != 0 || g->m->gcing != 0 || g->m->dying
!= 0)
With all these g->m I feel like it's worth writing
    mp = g->m;
and then using mp.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:771: // Install m and g (= m->g0) and set the stack
bounds
s/m and //

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:780: mp->curg->m = mp;
Shouldn't this line be in runtime·newextram?  Why is it here?

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
File src/pkg/runtime/sys_darwin_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
src/pkg/runtime/sys_darwin_386.s:252: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
File src/pkg/runtime/sys_darwin_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
src/pkg/runtime/sys_darwin_amd64.s:211: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
File src/pkg/runtime/sys_dragonfly_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
src/pkg/runtime/sys_dragonfly_386.s:217: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
File src/pkg/runtime/sys_dragonfly_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
src/pkg/runtime/sys_dragonfly_amd64.s:178: // g = m->signal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_386.s:199: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_amd64.s:199: // g = m->signal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_arm.s:216: // g = m->signal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_386.s:182: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_amd64.s:199: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_arm.s:308: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
File src/pkg/runtime/sys_nacl_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
src/pkg/runtime/sys_nacl_386.s:181: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
File src/pkg/runtime/sys_nacl_amd64p32.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
src/pkg/runtime/sys_nacl_amd64p32.s:274: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_386.s:210: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_amd64.s:228: // g = m->signal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_arm.s:221: // g = m->signal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
File src/pkg/runtime/sys_openbsd_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
src/pkg/runtime/sys_openbsd_386.s:190: // g = m->gsignal
s/m->/g->m->/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
File src/pkg/runtime/sys_openbsd_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
src/pkg/runtime/sys_openbsd_amd64.s:219: // g = m->signal
s/m->/g->m->/

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

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/vlop_arm....
src/pkg/runtime/vlop_arm.s:92: // Restore R1-R8 and R11-R12, but ignore the
saved R9 (m) and R10 (g).
This comment needs to be updated, and the code looks wrong.  It seems that it
should now restore R9 as well.

[dave_cheney.net, 2014-06-16 20:53:02]

Thanks Minux and rsc. Now R9 is free, we won't have to spill a register during
memmove on arm,
https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s

[bradfitz, 2014-06-16 20:55:04]

On Mon, Jun 16, 2014 at 1:53 PM, <dave@cheney.net> wrote:

> Thanks Minux and rsc. Now R9 is free, we won't have to spill a register
> during memmove on arm,
> https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s


So there will be with and without nacl variants of arm memmove*.s etc?

[iant, 2014-06-16 21:01:33]

On Mon, Jun 16, 2014 at 1:55 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
>
>
>
> On Mon, Jun 16, 2014 at 1:53 PM, <dave@cheney.net> wrote:
>>
>> Thanks Minux and rsc. Now R9 is free, we won't have to spill a register
>> during memmove on arm,
>> https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s
>
>
> So there will be with and without nacl variants of arm memmove*.s etc?

R9 will be free on all ARM systems.

Ian

[bradfitz, 2014-06-16 21:04:37]

On Mon, Jun 16, 2014 at 2:01 PM, Ian Lance Taylor <iant@golang.org> wrote:

> On Mon, Jun 16, 2014 at 1:55 PM, Brad Fitzpatrick <bradfitz@golang.org>
> wrote:
> >
> >
> >
> > On Mon, Jun 16, 2014 at 1:53 PM, <dave@cheney.net> wrote:
> >>
> >> Thanks Minux and rsc. Now R9 is free, we won't have to spill a register
> >> during memmove on arm,
> >>
> https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s
> >
> >
> > So there will be with and without nacl variants of arm memmove*.s etc?
>
> R9 will be free on all ARM systems.


From CL description: "This is important for NaCl, because NaCl ARM code
cannot use R9 at all."

[rsc, 2014-06-16 21:14:37]

On Mon, Jun 16, 2014 at 1:55 PM, Brad Fitzpatrick <bradfitz@golang.org>
wrote:

> On Mon, Jun 16, 2014 at 1:53 PM, <dave@cheney.net> wrote:
>
>> Thanks Minux and rsc. Now R9 is free, we won't have to spill a register
>> during memmove on arm,
>> https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s
>
>
> So there will be with and without nacl variants of arm memmove*.s etc?
>

If that's worthwhile, yes, and that's okay. Most assembly functions can
operate in fewer registers and can be written without R9, so the same
assembly would work everywhere. It's really just memmove that has been
problematic in that regard.

Russ

[minux, 2014-06-16 21:32:12]

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:675: MOVL	DX, 0(SP)
similarly to asm_amd64.s, we can use:
MOVL $0, 0(SP)
here.

we can also consolidate the two "MOVL g_m(BP), BP" above
and below, and move to after havem label.
probably not worth the trouble though.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:753: //MOVQ	g_m(BP), BP
On 2014/06/16 19:09:16, josharian wrote:
> Leftover debug code?
yes.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:761: XORQ	R8, R8
On 2014/06/16 19:09:16, josharian wrote:
> Why not `MOVQ $0, 0(SP)` on the next line and omit this instruction? R8 is not
> used again.
agreed

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:43: BL		runtime·save_gm(SB);
s/save_gm/save_g/g
s/load_gm/load_g/g

there are other occurrences in this file.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/asm_a...
File src/pkg/runtime/cgo/asm_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/asm_a...
src/pkg/runtime/cgo/asm_arm.s:21: BL	runtime·load_gm(SB)
s/load_gm/load_g/

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
File src/pkg/runtime/cgo/gcc_darwin_386.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_386.c:41: * Known to ../../../cmd/8l/obj.c:/468
it's now at:
../../../liblink/sym.c:/468

however, there is also a TODO to move them (ctxt->tlsoffset)
back into the linker.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_l...
File src/pkg/runtime/cgo/gcc_linux_arm.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_l...
src/pkg/runtime/cgo/gcc_linux_arm.c:58: extern void crosscall_arm1(void
(*fn)(void), void (*setg_gcc)(void*), void*, void*);
remove the last void* in the prototype, and name the other void* as g
for consistency.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_w...
File src/pkg/runtime/cgo/gcc_windows_386.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_w...
src/pkg/runtime/cgo/gcc_windows_386.c:56: "movl %1, 0(%%eax)\n"	// MOVL g, 0(FS)
is 0(FS) correct here?

shouldn't it be 0(tmp)(TLS*1) or something like that?

same for gcc_windows_amd64.s

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c#n...
src/pkg/runtime/panic.c:501: if(g->m->locks-g->m->softfloat != 0 ||
g->m->mallocing != 0 || g->m->throwing != 0 || g->m->gcing != 0 || g->m->dying
!= 0)
On 2014/06/16 20:32:34, iant wrote:
> With all these g->m I feel like it's worth writing
>     mp = g->m;
> and then using mp.
The reason I hesitated to introduce "M *m = g->m" throughout
is that if the scheduler preempt the goroutine after that
assignment, then the cached m might be incorrect.

I know that goroutine preemption only happens at function
entry for now, but if we introduce more finer-grained preemption
in the future, having such caching might pose very subtle problems.

so i prefer to use g->m as much as possible as long as the performance
doesn't suffer.

We probably should break this line to make it prettier instead.

I also suggest we make a note about this in the CL description.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:780: mp->curg->m = mp;
On 2014/06/16 20:32:34, iant wrote:
> Shouldn't this line be in runtime·newextram?  Why is it here?
yeah, please move this line to line 811, after "gp->status = Gsyscall;":
gp->m = mp;

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/runtime.h...
src/pkg/runtime/runtime.h:105: * 6l/linux rewrites 0(TLS) as -16(FS).
s/-16/-8/.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/softfloat...
File src/pkg/runtime/softfloat_arm.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/softfloat...
src/pkg/runtime/softfloat_arm.c:114: M *m;
perhaps add a comment here that m is locked in vlop_arm.s
so that caching g->m is safe here.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_arm.s:199: // first save R0, because runtime·load_gm
will clobber it
s/load_gm/load_g/g

(two occurrences.)

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_arm.s:291: // first save R0, because runtime·load_gm
will clobber it
s/load_gm/load_g/g

(two occurrences.)

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_arm.s:204: // first save R0, because runtime·load_gm
will clobber it
s/load_gm/load_g/g

(two occurrences.)

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

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/vlop_arm....
src/pkg/runtime/vlop_arm.s:92: // Restore R1-R8 and R11-R12, but ignore the
saved R9 (m) and R10 (g).
On 2014/06/16 20:32:35, iant wrote:
> This comment needs to be updated, and the code looks wrong.  It seems that it
> should now restore R9 as well.
you're right.

change line 98 to "MOVM.IA.W (R0), [R1-R9]".
or
#ifdef GOOS_nacl
   MOVM.IA.W (R0), [R1-R8] // R9 is read-only on NaCl.
#else
   MOVM.IA.W (R0, [R1-R9]
#endif

[minux, 2014-06-16 21:39:40]

On 2014/06/16 21:14:37, rsc wrote:
> On Mon, Jun 16, 2014 at 1:55 PM, Brad Fitzpatrick <mailto:bradfitz@golang.org>
> wrote:
> > On Mon, Jun 16, 2014 at 1:53 PM, <mailto:dave@cheney.net> wrote:
> >> Thanks Minux and rsc. Now R9 is free, we won't have to spill a register
> >> during memmove on arm,
> >> https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s
> > So there will be with and without nacl variants of arm memmove*.s etc?
> If that's worthwhile, yes, and that's okay. Most assembly functions can
> operate in fewer registers and can be written without R9, so the same
> assembly would work everywhere. It's really just memmove that has been
> problematic in that regard.
NaCl/ARM will always have NEON present, so I should leave the current memmove
as is (or enhance it to use R9, but that will only benefit pre-NEON systems),
and write a new implementation using NEON (where one can move 128 bytes without
saving registers [using q8-q15, or 64-byte if we want to preserve compatibility
with systems with fewer FP registers]). That should be faster than the current
version that only batch 32-byte (R1-R8) at a time.

[iant, 2014-06-16 23:07:05]

On Mon, Jun 16, 2014 at 2:04 PM, Brad Fitzpatrick <bradfitz@golang.org> wrote:
> On Mon, Jun 16, 2014 at 2:01 PM, Ian Lance Taylor <iant@golang.org> wrote:
>>
>> On Mon, Jun 16, 2014 at 1:55 PM, Brad Fitzpatrick <bradfitz@golang.org>
>> wrote:
>> >
>> >
>> >
>> > On Mon, Jun 16, 2014 at 1:53 PM, <dave@cheney.net> wrote:
>> >>
>> >> Thanks Minux and rsc. Now R9 is free, we won't have to spill a register
>> >> during memmove on arm,
>> >>
>> >> https://code.google.com/p/go/source/browse/src/pkg/runtime/memmove_arm.s
>> >
>> >
>> > So there will be with and without nacl variants of arm memmove*.s etc?
>>
>> R9 will be free on all ARM systems.
>
>
> From CL description: "This is important for NaCl, because NaCl ARM code
> cannot use R9 at all."

Oh yeah.  Sorry.

Ian

[dvyukov, 2014-06-17 17:34:20]

does not apply cleanly anymore:
$ hg clpatch 109050043
abort: codereview issue 109050043 is out of date: patch and recent changes
conflict (7d2e78c502ab->f2d2c6cc39a9)

[dvyukov, 2014-06-17 17:52:11]

FTR, malloc become 6.4% slower on my Core2 L9600:

Before:
BenchmarkMalloc8        50000000                40.8 ns/op
BenchmarkMalloc8        50000000                40.6 ns/op
BenchmarkMalloc8        50000000                40.9 ns/op
BenchmarkMalloc8        50000000                41.3 ns/op

After:
BenchmarkMalloc8        50000000                43.2 ns/op
BenchmarkMalloc8        50000000                43.2 ns/op
BenchmarkMalloc8        50000000                43.9 ns/op
BenchmarkMalloc8        50000000                44.7 ns/op

This can be up to 1% for real apps. Something we may want to recover later.

Race detector become 1% slower, not significant.

[dvyukov, 2014-06-18 17:03:13]

LGTM for the runtime changes

[rsc, 2014-06-18 17:09:03]

On Tue, Jun 17, 2014 at 10:52 AM, <dvyukov@google.com> wrote:

> FTR, malloc become 6.4% slower on my Core2 L9600:
>
> Before:
> BenchmarkMalloc8        50000000                40.8 ns/op
> BenchmarkMalloc8        50000000                40.6 ns/op
> BenchmarkMalloc8        50000000                40.9 ns/op
> BenchmarkMalloc8        50000000                41.3 ns/op
>
> After:
> BenchmarkMalloc8        50000000                43.2 ns/op
> BenchmarkMalloc8        50000000                43.2 ns/op
> BenchmarkMalloc8        50000000                43.9 ns/op
> BenchmarkMalloc8        50000000                44.7 ns/op
>
> This can be up to 1% for real apps. Something we may want to recover
> later.
>
> Race detector become 1% slower, not significant.
>

I believe that loading m into a local variable in a few select places will
address these, but I'd like to do that separately.

[dvyukov, 2014-06-18 18:40:20]

sure, that was beneficial even with separate g and m

On Wed, Jun 18, 2014 at 10:09 AM, Russ Cox <rsc@golang.org> wrote:
> On Tue, Jun 17, 2014 at 10:52 AM, <dvyukov@google.com> wrote:
>>
>> FTR, malloc become 6.4% slower on my Core2 L9600:
>>
>> Before:
>> BenchmarkMalloc8        50000000                40.8 ns/op
>> BenchmarkMalloc8        50000000                40.6 ns/op
>> BenchmarkMalloc8        50000000                40.9 ns/op
>> BenchmarkMalloc8        50000000                41.3 ns/op
>>
>> After:
>> BenchmarkMalloc8        50000000                43.2 ns/op
>> BenchmarkMalloc8        50000000                43.2 ns/op
>> BenchmarkMalloc8        50000000                43.9 ns/op
>> BenchmarkMalloc8        50000000                44.7 ns/op
>>
>> This can be up to 1% for real apps. Something we may want to recover
>> later.
>>
>> Race detector become 1% slower, not significant.
>
>
> I believe that loading m into a local variable in a few select places will
> address these, but I'd like to do that separately.

[rsc, 2014-06-24 20:08:21]

https://codereview.appspot.com/109050043/diff/40001/doc/asm.html
File doc/asm.html (right):

https://codereview.appspot.com/109050043/diff/40001/doc/asm.html#newcode152
doc/asm.html:152: (Exceptions: the <code>g</code> register renaming on ARM.)
On 2014/06/16 19:09:16, josharian wrote:
> s/Exceptions/Exception/

Done.

https://codereview.appspot.com/109050043/diff/40001/src/cmd/5a/lex.c
File src/cmd/5a/lex.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/cmd/5a/lex.c#newcode203
src/cmd/5a/lex.c:203: "g",		LREG,	10, // avoid unintentionally clobber m/g using
R9/R10
On 2014/06/16 20:32:33, iant wrote:
> s|m/g|g|
> s|R9/||

Done.

https://codereview.appspot.com/109050043/diff/40001/src/liblink/obj6.c
File src/liblink/obj6.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/liblink/obj6.c#newcod...
src/liblink/obj6.c:397: if(ctxt->tlsg == nil)
On 2014/06/16 20:32:34, iant wrote:
> It's not relevant for this CL, but I don't understand why these two lines do
not
> appear in obj8.c.

I think the answer is that they appear in obj5.c because Go TLS on ARM requires
them for cgo (in contrast to x86 where we do not need the symbol even for cgo),
and they appear here because of Elias's shared library work, which so far is
amd64 only.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s
File src/pkg/runtime/asm_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:70: // set up m and g "registers"
On 2014/06/16 20:32:34, iant wrote:
> s/m and g "registers"/g "register"/

I've been leaving comments referring to m as a variable or even as a "register",
thinking it is notational shorthand for g->m. This code does set up g->m.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:665: JEQ	2(PC) // TODO
On 2014/06/16 20:32:34, iant wrote:
> Looks like this should be
>     JEQ needm
> Looks like that would have worked in the original as well.  And you can drop
the
> MOVL $0, BP.

I'll try that in a followup. I don't want to touch the Windows instructions once
they are working.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_386.s...
src/pkg/runtime/asm_386.s:675: MOVL	DX, 0(SP)
On 2014/06/16 21:32:11, minux wrote:
> similarly to asm_amd64.s, we can use:
> MOVL $0, 0(SP)
> here.
> 
> we can also consolidate the two "MOVL g_m(BP), BP" above
> and below, and move to after havem label.
> probably not worth the trouble though.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64.s
File src/pkg/runtime/asm_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:750: JEQ	2(PC)
On 2014/06/16 20:32:34, iant wrote:
> This should be simple
>     JEQ needm
> and I don't see any need for the "MOVL $0, BP".

Will fix in a followup.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:753: //MOVQ	g_m(BP), BP
On 2014/06/16 21:32:11, minux wrote:
> On 2014/06/16 19:09:16, josharian wrote:
> > Leftover debug code?
> yes.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_amd64...
src/pkg/runtime/asm_amd64.s:761: XORQ	R8, R8
On 2014/06/16 21:32:11, minux wrote:
> On 2014/06/16 19:09:16, josharian wrote:
> > Why not `MOVQ $0, 0(SP)` on the next line and omit this instruction? R8 is
not
> > used again.
> agreed

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s
File src/pkg/runtime/asm_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:43: BL		runtime·save_gm(SB);
On 2014/06/16 21:32:11, minux wrote:
> s/save_gm/save_g/g
> s/load_gm/load_g/g
> 
> there are other occurrences in this file.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:559: // void setmg(G*); set m and g. for use by needm.
On 2014/06/16 20:32:34, iant wrote:
> s/m and //

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:668: // save_gm saves the g and m registers into
pthread-provided
On 2014/06/16 20:32:34, iant wrote:
> s/save_gm/save_g/
> 
> There are remaining calls to save_gm in the new code above.  Shouldn't those
> change to save_g?

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/asm_arm.s...
src/pkg/runtime/asm_arm.s:686: TEXT runtime·load_g(SB),NOSPLIT,$0
On 2014/06/16 20:32:34, iant wrote:
> Shouldn't the calls to load_gm above change to load_g?

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/asm_a...
File src/pkg/runtime/cgo/asm_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/asm_a...
src/pkg/runtime/cgo/asm_arm.s:21: BL	runtime·load_gm(SB)
On 2014/06/16 21:32:11, minux wrote:
> s/load_gm/load_g/

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
File src/pkg/runtime/cgo/gcc_darwin_386.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_386.c:41: * Known to ../../../cmd/8l/obj.c:/468
On 2014/06/16 21:32:11, minux wrote:
> it's now at:
> ../../../liblink/sym.c:/468
> 
> however, there is also a TODO to move them (ctxt->tlsoffset)
> back into the linker.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_386.c:85: }
On 2014/06/16 20:32:34, iant wrote:
> You've dropped the line
>     tofree[ntofree++] = k;

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
File src/pkg/runtime/cgo/gcc_darwin_amd64.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_amd64.c:28: * The linker and runtime hard-code
these constant offsets
On 2014/06/16 20:32:34, iant wrote:
> s/these constants offsets/this constant offset/

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_d...
src/pkg/runtime/cgo/gcc_darwin_amd64.c:29: * from %gs where we expect to find m
and g.
On 2014/06/16 20:32:34, iant wrote:
> s/m and //

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_l...
File src/pkg/runtime/cgo/gcc_linux_arm.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_l...
src/pkg/runtime/cgo/gcc_linux_arm.c:58: extern void crosscall_arm1(void
(*fn)(void), void (*setg_gcc)(void*), void*, void*);
On 2014/06/16 21:32:11, minux wrote:
> remove the last void* in the prototype, and name the other void* as g
> for consistency.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_w...
File src/pkg/runtime/cgo/gcc_windows_386.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/cgo/gcc_w...
src/pkg/runtime/cgo/gcc_windows_386.c:56: "movl %1, 0(%%eax)\n"	// MOVL g, 0(FS)
On 2014/06/16 21:32:11, minux wrote:
> is 0(FS) correct here?
> 
> shouldn't it be 0(tmp)(TLS*1) or something like that?
> 
> same for gcc_windows_amd64.s

I'm not worried about it. The comment is a comment, not executable.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c
File src/pkg/runtime/panic.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c#n...
src/pkg/runtime/panic.c:492: //byte g;
On 2014/06/16 20:32:34, iant wrote:
> These two commented out lines should be removed.  Perhaps keep a comment like
>   // note that global g points to gsignal

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c#n...
src/pkg/runtime/panic.c:501: if(g->m->locks-g->m->softfloat != 0 ||
g->m->mallocing != 0 || g->m->throwing != 0 || g->m->gcing != 0 || g->m->dying
!= 0)
On 2014/06/16 21:32:11, minux wrote:
> On 2014/06/16 20:32:34, iant wrote:
> > With all these g->m I feel like it's worth writing
> >     mp = g->m;
> > and then using mp.
> The reason I hesitated to introduce "M *m = g->m" throughout
> is that if the scheduler preempt the goroutine after that
> assignment, then the cached m might be incorrect.
> 
> I know that goroutine preemption only happens at function
> entry for now, but if we introduce more finer-grained preemption
> in the future, having such caching might pose very subtle problems.
> 
> so i prefer to use g->m as much as possible as long as the performance
> doesn't suffer.
> 
> We probably should break this line to make it prettier instead.
> 
> I also suggest we make a note about this in the CL description.

Keith's rewrite will take care of this, because he is switching to explicit
acquirem/releasem that inhibits preemption in between.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/panic.c#n...
src/pkg/runtime/panic.c:501: if(g->m->locks-g->m->softfloat != 0 ||
g->m->mallocing != 0 || g->m->throwing != 0 || g->m->gcing != 0 || g->m->dying
!= 0)
On 2014/06/16 20:32:34, iant wrote:
> With all these g->m I feel like it's worth writing
>     mp = g->m;
> and then using mp.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c
File src/pkg/runtime/proc.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:771: // Install m and g (= m->g0) and set the stack
bounds
On 2014/06/16 20:32:34, iant wrote:
> s/m and //

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/proc.c#ne...
src/pkg/runtime/proc.c:780: mp->curg->m = mp;
On 2014/06/16 21:32:11, minux wrote:
> On 2014/06/16 20:32:34, iant wrote:
> > Shouldn't this line be in runtime·newextram?  Why is it here?
> yeah, please move this line to line 811, after "gp->status = Gsyscall;":
> gp->m = mp;

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/runtime.h
File src/pkg/runtime/runtime.h (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/runtime.h...
src/pkg/runtime/runtime.h:105: * 6l/linux rewrites 0(TLS) as -16(FS).
On 2014/06/16 21:32:11, minux wrote:
> s/-16/-8/.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/softfloat...
File src/pkg/runtime/softfloat_arm.c (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/softfloat...
src/pkg/runtime/softfloat_arm.c:114: M *m;
On 2014/06/16 21:32:11, minux wrote:
> perhaps add a comment here that m is locked in vlop_arm.s
> so that caching g->m is safe here.

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
File src/pkg/runtime/sys_darwin_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
src/pkg/runtime/sys_darwin_386.s:252: // g = m->gsignal
On 2014/06/16 20:32:34, iant wrote:
> s/m->/g->m->/

I'd rather leave it; see earlier comment about treating 'm' as a defined
concept.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
File src/pkg/runtime/sys_darwin_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_darwi...
src/pkg/runtime/sys_darwin_amd64.s:211: // g = m->gsignal
On 2014/06/16 20:32:34, iant wrote:
> s/m->/g->m->/

same.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
File src/pkg/runtime/sys_dragonfly_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
src/pkg/runtime/sys_dragonfly_386.s:217: // g = m->gsignal
On 2014/06/16 20:32:34, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
File src/pkg/runtime/sys_dragonfly_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_drago...
src/pkg/runtime/sys_dragonfly_amd64.s:178: // g = m->signal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_386.s:199: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_amd64.s:199: // g = m->signal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
File src/pkg/runtime/sys_freebsd_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_arm.s:199: // first save R0, because runtime·load_gm
will clobber it
On 2014/06/16 21:32:11, minux wrote:
> s/load_gm/load_g/g
> 
> (two occurrences.)

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_freeb...
src/pkg/runtime/sys_freebsd_arm.s:216: // g = m->signal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_386.s:182: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_amd64.s:199: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
File src/pkg/runtime/sys_linux_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_arm.s:291: // first save R0, because runtime·load_gm
will clobber it
On 2014/06/16 21:32:11, minux wrote:
> s/load_gm/load_g/g
> 
> (two occurrences.)

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_linux...
src/pkg/runtime/sys_linux_arm.s:308: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
File src/pkg/runtime/sys_nacl_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
src/pkg/runtime/sys_nacl_386.s:181: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
File src/pkg/runtime/sys_nacl_amd64p32.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_nacl_...
src/pkg/runtime/sys_nacl_amd64p32.s:274: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_386.s:210: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_amd64.s:228: // g = m->signal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
File src/pkg/runtime/sys_netbsd_arm.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_arm.s:204: // first save R0, because runtime·load_gm
will clobber it
On 2014/06/16 21:32:11, minux wrote:
> s/load_gm/load_g/g
> 
> (two occurrences.)

Done.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_netbs...
src/pkg/runtime/sys_netbsd_arm.s:221: // g = m->signal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
File src/pkg/runtime/sys_openbsd_386.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
src/pkg/runtime/sys_openbsd_386.s:190: // g = m->gsignal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
File src/pkg/runtime/sys_openbsd_amd64.s (right):

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/sys_openb...
src/pkg/runtime/sys_openbsd_amd64.s:219: // g = m->signal
On 2014/06/16 20:32:35, iant wrote:
> s/m->/g->m->/

same

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

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/vlop_arm....
src/pkg/runtime/vlop_arm.s:92: // Restore R1-R8 and R11-R12, but ignore the
saved R9 (m) and R10 (g).
On 2014/06/16 21:32:11, minux wrote:
> On 2014/06/16 20:32:35, iant wrote:
> > This comment needs to be updated, and the code looks wrong.  It seems that
it
> > should now restore R9 as well.
> you're right.
> 
> change line 98 to "MOVM.IA.W (R0), [R1-R9]".
> or
> #ifdef GOOS_nacl
>    MOVM.IA.W (R0), [R1-R8] // R9 is read-only on NaCl.
> #else
>    MOVM.IA.W (R0, [R1-R9]
> #endif

nacl is your responsibility. this CL is just for m vs g->m.

https://codereview.appspot.com/109050043/diff/40001/src/pkg/runtime/vlop_arm....
src/pkg/runtime/vlop_arm.s:92: // Restore R1-R8 and R11-R12, but ignore the
saved R9 (m) and R10 (g).
On 2014/06/16 20:32:35, iant wrote:
> This comment needs to be updated, and the code looks wrong.  It seems that it
> should now restore R9 as well.

Done.

[rsc, 2014-06-24 20:08:30]

Hello minux@golang.org, josharian@gmail.com, iant@golang.org, dave@cheney.net,
bradfitz@golang.org, dvyukov@google.com (cc:
golang-codereviews@googlegroups.com),

Please take another look.

[iant, 2014-06-24 20:54:53]

LGTM

A bunch of files have "chunk mismatch" errors in codereview, but I assume they
are largely as before.

[minux, 2014-06-25 02:37:52]

so what was the problem for windows?
i will take another pass tonight.

[minux, 2014-06-25 06:54:28]

LGTM.

Thank you very much!

[dave_cheney.net, 2014-06-25 08:59:33]

There is mistake in one file that is obscured by a chunk mismatch, cgo/arm_asm.s

which leads to 

# cmd/go
crosscall2: call to external function runtime.load_gm
crosscall2: undefined: runtime.load_gm

This might be a reitveld bug for not accepting the new patch.

https://codereview.appspot.com/109050043/diff/60001/doc/asm.html
File doc/asm.html (right):

https://codereview.appspot.com/109050043/diff/60001/doc/asm.html#newcode348
doc/asm.html:348: through the value of an otherwise unused (as far as Go is
concerned) register in the MMU.
mmu ? should this be cpu ?

https://codereview.appspot.com/109050043/diff/60001/src/cmd/ld/lib.c
File src/cmd/ld/lib.c (right):

https://codereview.appspot.com/109050043/diff/60001/src/cmd/ld/lib.c#newcode247
src/cmd/ld/lib.c:247: tlsg = linklookup(ctxt, "runtime.tlsg", 0);
this normalisation of the symbol and the variable name is good.

[dave_cheney.net, 2014-06-25 09:31:13]

With the load_gm issue above fixed the build passes.

I didn't bother doing any benchmarks, it is what it is. 

Let's get this in.

[dave_cheney.net, 2014-06-25 09:31:23]

With the load_gm issue above fixed the build passes on linux/arm

I didn't bother doing any benchmarks, it is what it is. 

Let's get this in.

[rsc, 2014-06-26 15:15:15]

Thank you for testing on ARM. I hadn't gotten around to it yet.

The problem on Windows was the external thread thing that came up and was
fixed separately, combined with doing a memset(g, 0, sizeof *g) after
initializing g->m.

[rsc, 2014-06-26 15:54:49]

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

all: remove 'extern register M *m' from runtime

The runtime has historically held two dedicated values g (current goroutine)
and m (current thread) in 'extern register' slots (TLS on x86, real registers
backed by TLS on ARM).

This CL removes the extern register m; code now uses g->m.

On ARM, this frees up the register that formerly held m (R9).
This is important for NaCl, because NaCl ARM code cannot use R9 at all.

The Go 1 macrobenchmarks (those with per-op times >= 10 µs) are unaffected:

BenchmarkBinaryTree17              5491374955     5471024381     -0.37%
BenchmarkFannkuch11                4357101311     4275174828     -1.88%
BenchmarkGobDecode                 11029957       11364184       +3.03%
BenchmarkGobEncode                 6852205        6784822        -0.98%
BenchmarkGzip                      650795967      650152275      -0.10%
BenchmarkGunzip                    140962363      141041670      +0.06%
BenchmarkHTTPClientServer          71581          73081          +2.10%
BenchmarkJSONEncode                31928079       31913356       -0.05%
BenchmarkJSONDecode                117470065      113689916      -3.22%
BenchmarkMandelbrot200             6008923        5998712        -0.17%
BenchmarkGoParse                   6310917        6327487        +0.26%
BenchmarkRegexpMatchMedium_1K      114568         114763         +0.17%
BenchmarkRegexpMatchHard_1K        168977         169244         +0.16%
BenchmarkRevcomp                   935294971      914060918      -2.27%
BenchmarkTemplate                  145917123      148186096      +1.55%

Minux previous reported larger variations, but these were caused by
run-to-run noise, not repeatable slowdowns.

Actual code changes by Minux.
I only did the docs and the benchmarking.

LGTM=dvyukov, iant, minux
R=minux, josharian, iant, dave, bradfitz, dvyukov
CC=golang-codereviews
https://codereview.appspot.com/109050043

https://codereview.appspot.com/109050043/diff/60001/doc/asm.html
File doc/asm.html (right):

https://codereview.appspot.com/109050043/diff/60001/doc/asm.html#newcode348
doc/asm.html:348: through the value of an otherwise unused (as far as Go is
concerned) register in the MMU.
On 2014/06/25 08:59:32, dfc wrote:
> mmu ? should this be cpu ?

no, we mean mmu.
the segment shadow register is part of the memory management unit (in the cpu).

[gobot, 2014-06-26 15:56:35]

This CL appears to have broken the plan9-386-cnielsen builder.
See http://build.golang.org/log/88dfadf618aa51069214bfacebba45af4f7680d8

[0intro, 2014-06-26 17:01:59]

> This CL appears to have broken the plan9-386-cnielsen builder.
> See http://build.golang.org/log/88dfadf618aa51069214bfacebba45af4f7680d8

The cron job pulled from the Bell Labs replica database at the wrong time
(they are experiencing issues on their side) and many files got arbitrary
deleted on the Plan 9 builder.

I'll restore the last Fossil snapshot when I'll be back home.

-- 
David du Colombier

[0intro, 2014-06-26 21:52:51]

The Plan 9 builder is back online.

-- 
David du Colombier

[0intro, 2014-06-26 22:48:58]

It seems this change breaks the Plan 9 build.

term% cd /usr/go/src/pkg/time; go test -run TestIssue5745
time.test 625556: suicide: sys: trap: fault read addr=0x28c pc=0x0000edaf

term% acid 625556
/proc/625556/text:386 plan 9 executable
/sys/lib/acid/port
/sys/lib/acid/386
acid: stk()
runtime.sighandler()+0x1ef ?file?:0
runtime.sigtramp()+0x50 ?file?:0
0x3049ade8 ?file?:0
acid: asm(*PC)
runtime.sighandler+0x1ef 0x0000edaf	MOVL	0x28c(AX),AX
runtime.sighandler+0x1f5 0x0000edb5	MOVL	AX,0x0(SP)
runtime.sighandler+0x1f8 0x0000edb8	MOVL	0x38(FP),AX
runtime.sighandler+0x1fc 0x0000edbc	MOVL	AX,0x4(SP)
runtime.sighandler+0x200 0x0000edc0	MOVL	0x1c(SP),AX
runtime.sighandler+0x204 0x0000edc4	INCL	AX
runtime.sighandler+0x205 0x0000edc5	MOVL	AX,0x8(SP)
runtime.sighandler+0x209 0x0000edc9	CALL	runtime.memmove(SB)
runtime.sighandler+0x20e 0x0000edce	MOVL	0x3c(FP),BX
runtime.sighandler+0x212 0x0000edd2	MOVL	0x20(SP),DX
runtime.sighandler+0x216 0x0000edd6	MOVL	0x14(SP),CX
runtime.sighandler+0x21a 0x0000edda	MOVL	CX,0x78(BX)
runtime.sighandler+0x21d 0x0000eddd	MOVL	0x38(DX),CX
runtime.sighandler+0x220 0x0000ede0	MOVL	CX,0x8c(BX)
runtime.sighandler+0x226 0x0000ede6	MOVL	0x38(DX),AX
runtime.sighandler+0x229 0x0000ede9	CMPL	AX,$0x0
runtime.sighandler+0x22c 0x0000edec	JEQ	runtime.sighandler+0x24a(SB)
runtime.sighandler+0x22e 0x0000edee	MOVL	0x44(DX),BX
runtime.sighandler+0x231 0x0000edf1	ADDL	$0xfffffffc,BX
runtime.sighandler+0x234 0x0000edf4	MOVL	0x38(DX),CX
runtime.sighandler+0x237 0x0000edf7	MOVL	CX,0x0(BX)
runtime.sighandler+0x239 0x0000edf9	MOVL	BX,0x44(DX)
runtime.sighandler+0x23c 0x0000edfc	MOVL	$runtime.sigpanic(SB),CX
runtime.sighandler+0x241 0x0000ee01	MOVL	CX,0x38(DX)
runtime.sighandler+0x244 0x0000ee04	XORL	AX,AX
runtime.sighandler+0x246 0x0000ee06	ADDL	$0x30,SP
runtime.sighandler+0x249 0x0000ee09	RET
runtime.sighandler+0x24a 0x0000ee0a	JMP	runtime.sighandler+0x23c(SB)
runtime.sighandler+0x24c 0x0000ee0c	MOVL	CX,AX
runtime.sighandler+0x24e 0x0000ee0e	ANDL	$0x1,AX
acid: 
echo kill > /proc/625556/ctl

-- 
David du Colombier

[0intro, 2014-06-26 23:11:43]

Here is the builder log:

http://build.golang.org/log/63b14b2d639c13caafc41b597a835e1db6910c94

-- 
David du Colombier

[minux, 2014-06-26 23:14:28]

On Jun 26, 2014 4:11 PM, "David du Colombier" <0intro@gmail.com> wrote:
>
> Here is the builder log:
>
> http://build.golang.org/log/63b14b2d639c13caafc41b597a835e1db6910c94
judged by the tests that failsed it's very likely the signal handling code
(sigtramp)  is buggy.