code review 9223046: cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and u...

cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of argument pointer locations

With this change the compiler emits a bitmap for each function
covering its stack frame arguments area.  If an argument word
is known to contain a pointer, a bit is set.  The garbage
collector reads this information when scanning the stack by
frames and uses it to ignores locations known to not contain a
pointer.

[cshapiro1, 2013-05-08 02:22:10]

Hello golang-dev@googlegroups.com,

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

[bradfitz, 2013-05-08 03:35:06]

Impressive code/test ratio!

Update Issue nnnn?

Does this make anything better which you can demonstrate with some Go code?



On Tue, May 7, 2013 at 7:22 PM, <cshapiro@google.com> wrote:

> Reviewers: golang-dev1,
>
> Message:
> Hello golang-dev@googlegroups.com,
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of
> argument pointer locations
>
> Please review this at
https://codereview.appspot.**com/9223046/<https://codereview.appspot.com/9223...
>
> Affected files:
>   M src/cmd/5l/5.out.h
>   M src/cmd/5l/l.h
>   M src/cmd/5l/obj.c
>   M src/cmd/6l/6.out.h
>   M src/cmd/6l/l.h
>   M src/cmd/6l/obj.c
>   M src/cmd/6l/optab.c
>   M src/cmd/8l/8.out.h
>   M src/cmd/8l/l.h
>   M src/cmd/8l/obj.c
>   M src/cmd/8l/optab.c
>   A src/cmd/gc/bv.c
>   M src/cmd/gc/go.h
>   M src/cmd/gc/pgen.c
>   M src/cmd/ld/lib.c
>   M src/pkg/runtime/mgc0.c
>   M src/pkg/runtime/runtime.h
>   M src/pkg/runtime/symtab.c
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[cshapiro1, 2013-05-08 05:08:44]

I will update the change description with the relevant issue.

This infrastructure is currently hidden behind a disabled flag so it should
have no effect.  When the flag is enabled there are pointers ignored that
would otherwise be scanned.  However, it is hard to quantify how much this
"helps" as even with this change locations in the out area are scanned
before they are initialized.  At the moment, the benefit is largely a
function of how many garbage values are hanging around on your stack.

A forthcoming change will extend the bitmap to cover all locals and be
liveness precise.  That is when we will see a real benefit.  It will also
be much easier to empirically validate correctness.

On Tue, May 7, 2013 at 8:35 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Impressive code/test ratio!
>
> Update Issue nnnn?
>
> Does this make anything better which you can demonstrate with some Go code?
>
>
>
> On Tue, May 7, 2013 at 7:22 PM, <cshapiro@google.com> wrote:
>
>> Reviewers: golang-dev1,
>>
>> Message:
>> Hello golang-dev@googlegroups.com,
>>
>> I'd like you to review this change to
>> https://code.google.com/p/go/
>>
>>
>> Description:
>> cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of
>> argument pointer locations
>>
>> Please review this at
https://codereview.appspot.**com/9223046/<https://codereview.appspot.com/9223...
>>
>> Affected files:
>>   M src/cmd/5l/5.out.h
>>   M src/cmd/5l/l.h
>>   M src/cmd/5l/obj.c
>>   M src/cmd/6l/6.out.h
>>   M src/cmd/6l/l.h
>>   M src/cmd/6l/obj.c
>>   M src/cmd/6l/optab.c
>>   M src/cmd/8l/8.out.h
>>   M src/cmd/8l/l.h
>>   M src/cmd/8l/obj.c
>>   M src/cmd/8l/optab.c
>>   A src/cmd/gc/bv.c
>>   M src/cmd/gc/go.h
>>   M src/cmd/gc/pgen.c
>>   M src/cmd/ld/lib.c
>>   M src/pkg/runtime/mgc0.c
>>   M src/pkg/runtime/runtime.h
>>   M src/pkg/runtime/symtab.c
>>
>>
>> --
>>
>> ---You received this message because you are subscribed to the Google
>> Groups "golang-dev" group.
>> To unsubscribe from this group and stop receiving emails from it, send an
>> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>> .
>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>> .
>>
>>
>>
>

[bradfitz, 2013-05-08 05:12:01]

Cool, thanks for the background.  Putting some of that in the issue and/or
CL description would be helpful.



On Tue, May 7, 2013 at 10:08 PM, Carl Shapiro <cshapiro@google.com> wrote:

> I will update the change description with the relevant issue.
>
> This infrastructure is currently hidden behind a disabled flag so it
> should have no effect.  When the flag is enabled there are pointers ignored
> that would otherwise be scanned.  However, it is hard to quantify how much
> this "helps" as even with this change locations in the out area are scanned
> before they are initialized.  At the moment, the benefit is largely a
> function of how many garbage values are hanging around on your stack.
>
> A forthcoming change will extend the bitmap to cover all locals and be
> liveness precise.  That is when we will see a real benefit.  It will also
> be much easier to empirically validate correctness.
>
> On Tue, May 7, 2013 at 8:35 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>
>> Impressive code/test ratio!
>>
>> Update Issue nnnn?
>>
>> Does this make anything better which you can demonstrate with some Go
>> code?
>>
>>
>>
>> On Tue, May 7, 2013 at 7:22 PM, <cshapiro@google.com> wrote:
>>
>>> Reviewers: golang-dev1,
>>>
>>> Message:
>>> Hello golang-dev@googlegroups.com,
>>>
>>> I'd like you to review this change to
>>> https://code.google.com/p/go/
>>>
>>>
>>> Description:
>>> cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of
>>> argument pointer locations
>>>
>>> Please review this at
https://codereview.appspot.**com/9223046/<https://codereview.appspot.com/9223...
>>>
>>> Affected files:
>>>   M src/cmd/5l/5.out.h
>>>   M src/cmd/5l/l.h
>>>   M src/cmd/5l/obj.c
>>>   M src/cmd/6l/6.out.h
>>>   M src/cmd/6l/l.h
>>>   M src/cmd/6l/obj.c
>>>   M src/cmd/6l/optab.c
>>>   M src/cmd/8l/8.out.h
>>>   M src/cmd/8l/l.h
>>>   M src/cmd/8l/obj.c
>>>   M src/cmd/8l/optab.c
>>>   A src/cmd/gc/bv.c
>>>   M src/cmd/gc/go.h
>>>   M src/cmd/gc/pgen.c
>>>   M src/cmd/ld/lib.c
>>>   M src/pkg/runtime/mgc0.c
>>>   M src/pkg/runtime/runtime.h
>>>   M src/pkg/runtime/symtab.c
>>>
>>>
>>> --
>>>
>>> ---You received this message because you are subscribed to the Google
>>> Groups "golang-dev" group.
>>> To unsubscribe from this group and stop receiving emails from it, send
>>> an email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>>> .
>>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>>> .
>>>
>>>
>>>
>>
>

[cshapiro1, 2013-05-08 05:28:23]

Will do.  Thanks.


On Tue, May 7, 2013 at 10:11 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:

> Cool, thanks for the background.  Putting some of that in the issue and/or
> CL description would be helpful.
>
>
>
> On Tue, May 7, 2013 at 10:08 PM, Carl Shapiro <cshapiro@google.com> wrote:
>
>> I will update the change description with the relevant issue.
>>
>> This infrastructure is currently hidden behind a disabled flag so it
>> should have no effect.  When the flag is enabled there are pointers ignored
>> that would otherwise be scanned.  However, it is hard to quantify how much
>> this "helps" as even with this change locations in the out area are scanned
>> before they are initialized.  At the moment, the benefit is largely a
>> function of how many garbage values are hanging around on your stack.
>>
>> A forthcoming change will extend the bitmap to cover all locals and be
>> liveness precise.  That is when we will see a real benefit.  It will also
>> be much easier to empirically validate correctness.
>>
>> On Tue, May 7, 2013 at 8:35 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote:
>>
>>> Impressive code/test ratio!
>>>
>>> Update Issue nnnn?
>>>
>>> Does this make anything better which you can demonstrate with some Go
>>> code?
>>>
>>>
>>>
>>> On Tue, May 7, 2013 at 7:22 PM, <cshapiro@google.com> wrote:
>>>
>>>> Reviewers: golang-dev1,
>>>>
>>>> Message:
>>>> Hello golang-dev@googlegroups.com,
>>>>
>>>> I'd like you to review this change to
>>>> https://code.google.com/p/go/
>>>>
>>>>
>>>> Description:
>>>> cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of
>>>> argument pointer locations
>>>>
>>>> Please review this at
https://codereview.appspot.**com/9223046/<https://codereview.appspot.com/9223...
>>>>
>>>> Affected files:
>>>>   M src/cmd/5l/5.out.h
>>>>   M src/cmd/5l/l.h
>>>>   M src/cmd/5l/obj.c
>>>>   M src/cmd/6l/6.out.h
>>>>   M src/cmd/6l/l.h
>>>>   M src/cmd/6l/obj.c
>>>>   M src/cmd/6l/optab.c
>>>>   M src/cmd/8l/8.out.h
>>>>   M src/cmd/8l/l.h
>>>>   M src/cmd/8l/obj.c
>>>>   M src/cmd/8l/optab.c
>>>>   A src/cmd/gc/bv.c
>>>>   M src/cmd/gc/go.h
>>>>   M src/cmd/gc/pgen.c
>>>>   M src/cmd/ld/lib.c
>>>>   M src/pkg/runtime/mgc0.c
>>>>   M src/pkg/runtime/runtime.h
>>>>   M src/pkg/runtime/symtab.c
>>>>
>>>>
>>>> --
>>>>
>>>> ---You received this message because you are subscribed to the Google
>>>> Groups "golang-dev" group.
>>>> To unsubscribe from this group and stop receiving emails from it, send
>>>> an email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
>>>> .
>>>> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
>>>> .
>>>>
>>>>
>>>>
>>>
>>
>

[DMorsing, 2013-05-08 09:04:55]

https://codereview.appspot.com/9223046/diff/8001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/8001/src/cmd/gc/pgen.c#newcode283
src/cmd/gc/pgen.c:283: stackmap(Node *fn)
With the addition of the TYPE instruction, it should be possible to do this
entirely in the linker, without having to deal with which fields are inargs or
outargs. It is also done after allocauto, so the final locations of the stack
variables will also be determined.

The compiler will have to be modified to put out liveness info at some point,
but might as well build on the stuff that's already there.

[dvyukov, 2013-05-08 18:14:25]

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:239: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1);
is it meant to be sizeof(**fgc)?

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:239: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1);
I am a bit concerned about lots of small and markable allocations.
How many can be there?
Can we allocate it from a big FlagNoGC buffer or construct statically?

[cshapiro1, 2013-05-08 18:25:16]

https://codereview.appspot.com/9223046/diff/8001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/8001/src/cmd/gc/pgen.c#newcode283
src/cmd/gc/pgen.c:283: stackmap(Node *fn)
I started out trying to implement this at link time.  It turned out to be more
difficult than I had hoped.  Many of the data structured needed (for example,
the type information data structures) are not readily available at link time.  I
could unpack all of the information being sent to the linker, but that  turned
out to be a lot more work than I would have liked.  

Right now I am just generating information for the argument list but my next
change will be adding information for locals.  It looks like the same data
structure will be used for locals and for the argument list and its simpler to
compute it all at once.  Doing that work at link time would require recomputing 
a lot of state available at compile time and teaching the linker about data
structures known to the compiler.

[cshapiro1, 2013-05-08 18:30:30]

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:239: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1);
Yes, that is my near-term plan.

This information should be suitable for allocation in a read only segment of the
executable.  Moreoever, I expect there to be a lot of repeated data and some
space will be saved through coalescing.

I am defering any effort to efficiently store this until I have finalized the
representation of the pointer descriptions.  I keep discovering corner cases
that influence how things should be done.

In the mean time, if you think it would be helpful, I can put a TODO here.

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:239: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1);
Yes, you are right.  I'll fix that shortly.

[dvyukov, 2013-05-09 09:18:05]

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:239: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1);
On 2013/05/08 18:30:31, cshapiro1 wrote:
> Yes, that is my near-term plan.
> 
> This information should be suitable for allocation in a read only segment of
the
> executable.  Moreoever, I expect there to be a lot of repeated data and some
> space will be saved through coalescing.
> 
> I am defering any effort to efficiently store this until I have finalized the
> representation of the pointer descriptions.  I keep discovering corner cases
> that influence how things should be done.
> 
> In the mean time, if you think it would be helpful, I can put a TODO here.

TODO is fine

[cshapiro1, 2013-05-09 21:02:49]

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/8001/src/pkg/runtime/symtab.c#new...
src/pkg/runtime/symtab.c:239: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1);
Thanks.  Done.

[khr, 2013-05-09 21:49:53]

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/go.h#newcode286
src/cmd/gc/go.h:286: Bvec*	bv;
This should be called "arg_pointer_map" or something, not just "bv".

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/pgen.c#newcode175
src/cmd/gc/pgen.c:175: walktype1(Type *t, vlong *xoffset, Bvec *bv)
This code allocates slots for the arguments.  We must already have such code
somewhere, right?  Shouldn't we be sharing that code instead of duplicating it?

https://codereview.appspot.com/9223046/diff/22001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/22001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1411: if(f->args > 0) {
Shouldn't there be some agreement between f->args and f->ptrs.len that we can
check?

[cshapiro1, 2013-05-09 22:00:39]

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/go.h#newcode286
src/cmd/gc/go.h:286: Bvec*	bv;
I am normally all in favor of long, descriptive names.  That does not seem to be
the style in this code, nor use the use of underscores in variable names.  I'll
rename "bv" to the more descriptive "stackmap" which should be somewhat less
cryptic.  Let me know if you think otherwise.

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/pgen.c#newcode175
src/cmd/gc/pgen.c:175: walktype1(Type *t, vlong *xoffset, Bvec *bv)
I am not sure I fully understand what you mean by allocating slots for
arguments.  The way I think about this code is it sets a bit in bv for each
pointer value offset in t.

I contemplated writing a call-back driven interface for this code. 
Unfortunately, there are a lot of one offs for this sort of thing and so either
way I was going to end up with one more way do walk a type.  I just discovered
an iterator type for structs which would allow at least the bottom-most case to
be eliminated.

There is certainly an opportunity for a clean-up.

https://codereview.appspot.com/9223046/diff/22001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/22001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1411: if(f->args > 0) {
I would add an assert but we don't do asserts.  I can check the condition a
compile time.  What do you think?

[khr1, 2013-05-09 22:26:37]

On Thu, May 9, 2013 at 3:00 PM, <cshapiro@google.com> wrote:

>
>
https://codereview.appspot.**com/9223046/diff/22001/src/**cmd/gc/go.h<https:/...
> File src/cmd/gc/go.h (right):
>
> https://codereview.appspot.**com/9223046/diff/22001/src/**
>
cmd/gc/go.h#newcode286<https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/go.h#newcode286>
> src/cmd/gc/go.h:286: Bvec*      bv;
> I am normally all in favor of long, descriptive names.  That does not
> seem to be the style in this code, nor use the use of underscores in
> variable names.  I'll rename "bv" to the more descriptive "stackmap"
> which should be somewhat less cryptic.  Let me know if you think
> otherwise.
>
> That's better.  I was somewhat confused while reading that this is a map
of the
argument list, not a map of the locals.  So maybe "argmap"?

>
>
https://codereview.appspot.**com/9223046/diff/22001/src/**cmd/gc/pgen.c<https...
> File src/cmd/gc/pgen.c (right):
>
> https://codereview.appspot.**com/9223046/diff/22001/src/**
>
cmd/gc/pgen.c#newcode175<https://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/pgen.c#newcode175>
> src/cmd/gc/pgen.c:175: walktype1(Type *t, vlong *xoffset, Bvec *bv)
> I am not sure I fully understand what you mean by allocating slots for
> arguments.  The way I think about this code is it sets a bit in bv for
> each pointer value offset in t.
>
> I contemplated writing a call-back driven interface for this code.
> Unfortunately, there are a lot of one offs for this sort of thing and so
> either way I was going to end up with one more way do walk a type.  I
> just discovered an iterator type for structs which would allow at least
> the bottom-most case to be eliminated.
>
> There is certainly an opportunity for a clean-up.
>
>  There has to be code somewhere in the compiler/linker that decides at
what offset
each argument goes.  You're just duplicating that logic.

>
> https://codereview.appspot.**com/9223046/diff/22001/src/**
>
pkg/runtime/mgc0.c<https://codereview.appspot.com/9223046/diff/22001/src/pkg/runtime/mgc0.c>
> File src/pkg/runtime/mgc0.c (right):
>
> https://codereview.appspot.**com/9223046/diff/22001/src/**
>
pkg/runtime/mgc0.c#newcode1411<https://codereview.appspot.com/9223046/diff/22001/src/pkg/runtime/mgc0.c#newcode1411>
> src/pkg/runtime/mgc0.c:1411: if(f->args > 0) {
> I would add an assert but we don't do asserts.  I can check the
> condition a compile time.  What do you think?
>
Check it at runtime and runtime·throw if it is bad.

>
>
https://codereview.appspot.**com/9223046/<https://codereview.appspot.com/9223...
>

[cshapiro1, 2013-05-09 22:35:54]

On Thu, May 9, 2013 at 3:26 PM, Keith Randall <khr@google.com> wrote:

> That's better.  I was somewhat confused while reading that this is a map
> of the
> argument list, not a map of the locals.  So maybe "argmap"?
>

At the moment the map covers only the argument area.  That will change and
this map will cover arguments and locals.  The type will also change
slightly as a new map needs to be generated at each call instruction but
the name can certainly stay the same.


>  There has to be code somewhere in the compiler/linker that decides at
> what offset
> each argument goes.  You're just duplicating that logic.
>

Strinctly speaking, I believe this statement is true.  However, if we are
thinking about the same code that assigns offsets, that code is much more
general and complicated.  Among other things, it needs to handle the case
of type information not yet known.


> Check it at runtime and runtime·throw if it is bad.
>

 Will do.  I will re-upload shortly.

[cshapiro1, 2013-05-10 00:52:08]

On 2013/05/09 22:26:37, khr1 wrote:
> Check it at runtime and runtime·throw if it is bad.

I forgot that the ptrs.len is the number of 32-bit words of the bitmap and is
equal to or greater than the number of argument bytes.  I can ensure
"f->args/sizeof(uintptr) <= f->ptrs.len*32" though that is unsatisfying as most
procedures have less than 32 words of arguments.

[cshapiro1, 2013-05-20 20:39:32]

Ping?

[dvyukov, 2013-05-21 06:55:55]

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

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:813: #pragma	varargck	type	"a"	Slice
please send this is a separate CL

[dvyukov, 2013-05-21 07:21:40]

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1405: if(f->locals == 0 || *(bool*)doframe == true)
add a comment what this condition means

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1409: addroot((Obj){sp + outs, f->locals, 0});
add a comment here and for other addroot() parts what exactly parts we scan --
args, out args, locals

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1412: if(f->ptrs.array != nil) {
when does array==nil possible?
do we generate info not for all functions?

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1415: for(i = 0; i < f->ptrs.len; ++i) {
i++

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:240: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(**fgc), FlagNoPointers, 0, 1);
add a check that args is consistent with nptrs, otherwise GC will break later

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:240: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(**fgc), FlagNoPointers, 0, 1);
just allocate it with FlagNoPointers|FlagNoGC, then you do not need the global
fgc array at all

[cshapiro1, 2013-05-21 23:08:56]

PTAL

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1405: if(f->locals == 0 || *(bool*)doframe == true)
This is actually documented in the comment above the function.  Are you
suggesting that I move the comment down to this line?

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1409: addroot((Obj){sp + outs, f->locals, 0});
On 2013/05/21 07:21:40, dvyukov wrote:
> add a comment here and for other addroot() parts what exactly parts we scan --
> args, out args, locals

Done.

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1412: if(f->ptrs.array != nil) {
At the moment there are many classes of functions (assembly routines, varargs
routines, etc.) that do not have any pointer information available.

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1415: for(i = 0; i < f->ptrs.len; ++i) {
On 2013/05/21 07:21:40, dvyukov wrote:
> i++

Done.

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

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:813: #pragma	varargck	type	"a"	Slice
On 2013/05/21 06:55:55, dvyukov wrote:
> please send this is a separate CL

Done.

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:240: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(**fgc), FlagNoPointers, 0, 1);
Good idea, done.

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:240: fgc[nfunc-1] =
runtime·mallocgc(sym->value*sizeof(**fgc), FlagNoPointers, 0, 1);
The best I can do is ensure that nptrs <= args/widthptr.  I have added such a
check.

[iant, 2013-05-22 00:41:00]

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c
File src/cmd/5l/obj.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c#newcode632
src/cmd/5l/obj.c:632: goto casdef;
s/casdef/casedef/

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c#newcode640
src/cmd/5l/obj.c:640: goto casdef;
s/casdef/casedef/

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c#newcode641
src/cmd/5l/obj.c:641: cursym->ptrs[p->from.offset] = p->to.offset;
In general we want to avoid crashing if somebody sneaks in an invalid .5 file,
for increased reliability on the playground and other places where people
compile Go code.  So I think you should check for p->from.offset >=
cursym->nptrs.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/obj.c
File src/cmd/6l/obj.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/obj.c#newcode630
src/cmd/6l/obj.c:630: cursym->ptrs[p->from.offset] = p->to.offset;
Check for p->from.offset >= cursym->nptrs.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/optab.c
File src/cmd/6l/optab.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/optab.c#newcode1340
src/cmd/6l/optab.c:1340: { ANPTRS },
Do these need to be in 5l/optab.c, or does that work differently?

https://codereview.appspot.com/9223046/diff/51001/src/cmd/8l/obj.c
File src/cmd/8l/obj.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/8l/obj.c#newcode640
src/cmd/8l/obj.c:640: cursym->ptrs[p->from.offset] = p->to.offset;
Check for p->from.offset >= cursym->nptrs.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c
File src/cmd/gc/bv.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c#newcode12
src/cmd/gc/bv.c:12: WORDBITS = WORDSIZE << WORDSIZELOG2,
I may be misreading, but WORDSIZE == 4, so this makes WORDBITS == 4 << 2 == 16. 
Shouldn't it be 32?

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c#newcode16
src/cmd/gc/bv.c:16: size(uintptr n)
s/size/bvsize/

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c#newcode69
src/cmd/gc/bv.c:69: return (word ? 1 : 0);
I think the style is to omit the parentheses here.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/go.h#newcode703
src/cmd/gc/go.h:703: uint32	b[];
As far as I know we don't currently use C99 flexible array members anywhere else
in the toolchain.  I guess it should be OK by now, though.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode224
src/cmd/gc/pgen.c:224: // struct { Itab* tab;  void* data; }
This comment is slightly misleading as the data field is not necessarily a
pointer.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode234
src/cmd/gc/pgen.c:234: if(t->bound < -1)
I don't think you should write this check here.  If you want to check, in the
!isslice case, that t->bound >= 0, that seems fine.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode237
src/cmd/gc/pgen.c:237: // struct { byte* array; uint32 len; uint32 cap; }
s/uint32/int/g
The len and cap fields have Go type int now.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode275
src/cmd/gc/pgen.c:275: roundup(int32 x, int32 n)
I think you can just use rnd, defined in align.c, rather than defining your own
version.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode305
src/cmd/gc/pgen.c:305: bv = bvalloc((thiswidth + inargwidth + outargwidth) /
widthptr);
Here you create the bitvector with the total of the widths.  But when you walk
over the types, you restart each walk at offset 0.  Shouldn't the walks start
from some offset?  Right now it seems like you may set the same bits several
times.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode312
src/cmd/gc/pgen.c:312: if(bvpopcnt(bv) == 0) {
If this is the only use of bvpopcnt then I think it would be better to have a
bvempty or something that just looks for all zero values.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode323
src/cmd/gc/pgen.c:323: prog->from.offset = i;
Here you are setting the offset as a multiple of 32, but in the linker you are
taking the offset as a constant.  Is that right?

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

https://codereview.appspot.com/9223046/diff/51001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:415: Slice	ptrs;	// pointer map
When you change this struct you should also change Func in extern.go in this
directory.

https://codereview.appspot.com/9223046/diff/51001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:236:
//if(func[nfunc-1].args==ArgsSizeUnknown)runtime·printf("unknown:
%S\n",func[nfunc-1].name);
If you want to keep this, break the line before the function call.

[cshapiro1, 2013-05-22 06:35:21]

PTAL

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c
File src/cmd/5l/obj.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c#newcode632
src/cmd/5l/obj.c:632: goto casdef;
Done.  I will submit a clean-up after this change.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c#newcode640
src/cmd/5l/obj.c:640: goto casdef;
Ditto.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/5l/obj.c#newcode641
src/cmd/5l/obj.c:641: cursym->ptrs[p->from.offset] = p->to.offset;
I wrote an expanded set of checks that addresses this comment as well as those
from Dmitry.  They will appear in my next upload.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/obj.c
File src/cmd/6l/obj.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/obj.c#newcode630
src/cmd/6l/obj.c:630: cursym->ptrs[p->from.offset] = p->to.offset;
Ditto.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/optab.c
File src/cmd/6l/optab.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/6l/optab.c#newcode1340
src/cmd/6l/optab.c:1340: { ANPTRS },
This is a style difference.  I ran into this issue when adding ALOCALS and
concluded that it had to be done this way.  The 5 compilers are much more fussy
about instructions than the 6 and 8 computers.

I think the 5 compilers should not be different in this regard.   I will file a
issue for this.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/8l/obj.c
File src/cmd/8l/obj.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/8l/obj.c#newcode640
src/cmd/8l/obj.c:640: cursym->ptrs[p->from.offset] = p->to.offset;
Ditto.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c
File src/cmd/gc/bv.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c#newcode12
src/cmd/gc/bv.c:12: WORDBITS = WORDSIZE << WORDSIZELOG2,
You are right.  I have revised these definitions.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c#newcode16
src/cmd/gc/bv.c:16: size(uintptr n)
Sure, renamed.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/bv.c#newcode69
src/cmd/gc/bv.c:69: return (word ? 1 : 0);
Parenthesis are gone.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/go.h
File src/cmd/gc/go.h (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/go.h#newcode703
src/cmd/gc/go.h:703: uint32	b[];
I am happy to remove this and have the variable length data follow n, or make b
a 0 element array.  What would you prefer?

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode224
src/cmd/gc/pgen.c:224: // struct { Itab* tab;  void* data; }
These definitions come from runtime.h and a void* is used there as well.  I will
add a union definition in the comment for clarity.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode234
src/cmd/gc/pgen.c:234: if(t->bound < -1)
The case I am guarding against is negative values of bound that are not -1. 
Types are initialized with a bound of -100 to indicate that the type needs some
values filled in.  The definition of isslice, unfortunately, assumes that any
negative value indicates a slice, which confuses incomplete types to with
slices.

This is subtle.  I will add a comment.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode237
src/cmd/gc/pgen.c:237: // struct { byte* array; uint32 len; uint32 cap; }
Thanks.  I have updated the comment here and below TSTRING as well.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode275
src/cmd/gc/pgen.c:275: roundup(int32 x, int32 n)
I do not think rnd is useful here.  With rnd you can rounding to power of 2
boundary between 1 and 8.  All other values will signal an error at runtime.  My
initial intuition was to use rnd.  I defined my own rounding function to work
around that limitation.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode305
src/cmd/gc/pgen.c:305: bv = bvalloc((thiswidth + inargwidth + outargwidth) /
widthptr);
This is very subtle.  The offset is primed in the structure case starting on
line 252.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode312
src/cmd/gc/pgen.c:312: if(bvpopcnt(bv) == 0) {
Sure, right now bvpopcnt is overkill.  I defined it so I can count the number of
1 bits and short circuit emitting pointer map data when there is a long run of
0s at the end of the bit vector.  I have replaced bvpopcnt with bvisempty for
the time being.

https://codereview.appspot.com/9223046/diff/51001/src/cmd/gc/pgen.c#newcode323
src/cmd/gc/pgen.c:323: prog->from.offset = i;
You are right.  That is what I get for having many pending changes in one
client.

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

https://codereview.appspot.com/9223046/diff/51001/src/pkg/runtime/runtime.h#n...
src/pkg/runtime/runtime.h:415: Slice	ptrs;	// pointer map
Thanks.  I did that, actually, but neglected to add the file.  extern.go is now
part of the changelist.

https://codereview.appspot.com/9223046/diff/51001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/51001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:236:
//if(func[nfunc-1].args==ArgsSizeUnknown)runtime·printf("unknown:
%S\n",func[nfunc-1].name);
This is clutter let over from an experiment.  I have removed it.

[dvyukov, 2013-05-22 07:07:13]

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/38001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1405: if(f->locals == 0 || *(bool*)doframe == true)
On 2013/05/21 23:08:57, cshapiro1 wrote:
> This is actually documented in the comment above the function.  Are you
> suggesting that I move the comment down to this line?

Ah, OK

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1397: // locals need to be scanned.
But you also scan args in this case?
I am just trying to understand how exactly it works.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1425: for((j = (rem < 32) ? rem : 32); j > 0; j--) {
I guess we will need BSF intrinsic in future... just thinking aloud

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1426: if(w & b) {
drop {}

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1434: } else {
drop {}

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:238: if (sym->value >
func[nfunc-1].args/sizeof(uintptr)) {
>The best I can do is ensure that nptrs <= args/widthptr
do you mean that .ptrs does not cover the tail of args if there are no pointers?

each .ptrs value covers 32*sizeof(uintptr) of arg space.
the check seems to imply that it covers only sizeof(uintptr).

[DMorsing, 2013-05-22 09:29:47]

https://codereview.appspot.com/9223046/diff/62001/src/cmd/5g/peep.c
File src/cmd/5g/peep.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/cmd/5g/peep.c#newcode82
src/cmd/5g/peep.c:82: case APTRS:
Add these 5g changes to 6g and 8g as well.

https://codereview.appspot.com/9223046/diff/62001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/cmd/gc/pgen.c#newcode309
src/cmd/gc/pgen.c:309: if(thistype != nil)
This could be done more cleanly by using the fn->dcl list. Move the pointermap
call to after allocauto and it can be generalized to handle local variables as
well.

[cshapiro1, 2013-05-22 23:19:10]

PTAL

https://codereview.appspot.com/9223046/diff/62001/src/cmd/5g/peep.c
File src/cmd/5g/peep.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/cmd/5g/peep.c#newcode82
src/cmd/5g/peep.c:82: case APTRS:
Agreed.  Done.

https://codereview.appspot.com/9223046/diff/62001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/cmd/gc/pgen.c#newcode309
src/cmd/gc/pgen.c:309: if(thistype != nil)
I started by using the fn->dcl list but I had difficulty getting complete
information in all cases.  I believe the issue was with retrieving the this
pointer type but I might be misremembering.

You are absolutely right about doing the analysis after allocauto.  That is what
my next change, which integrates local variables, does.  This code will look
different after that as the pointer map computation will be driven by a data
flow based liveness computation.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1397: // locals need to be scanned.
Yes, when *doframe is true we scan everything between the SP and the (virtual)
FP.  This has the effect of scanning the caller's locals area and the arguments
area for the callee.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1425: for((j = (rem < 32) ? rem : 32); j > 0; j--) {
I have been thinking the same thing.  Compiler intrinsics for all the common bit
operations will be helpful.  I suspect other parts of the GC are performance
limited by bitmap scanning.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1426: if(w & b) {
Done.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1434: } else {
Done.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:238: if (sym->value >
func[nfunc-1].args/sizeof(uintptr)) {
Sorry, there is some units confusion here.  I have changed .nptrs to
consistently be the number of bits in the map.  So, this check does what it is
supposed to.

[dvyukov, 2013-05-23 05:28:58]

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1397: // locals need to be scanned.
On 2013/05/22 23:19:11, cshapiro1 wrote:
> Yes, when *doframe is true we scan everything between the SP and the (virtual)
> FP.  This has the effect of scanning the caller's locals area and the
arguments
> area for the callee.

You "Scan the entire stack frame" and then you additionally scan args in
"if(f->args > 0) {".
What is true?
1. The comment is incorrect.
2. The code under "if" is not supposed to be executed in this case.
3. Args is not a part of the entire stack frame.
4. I am missing something.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1425: for((j = (rem < 32) ? rem : 32); j > 0; j--) {
On 2013/05/22 23:19:11, cshapiro1 wrote:
> I have been thinking the same thing.  Compiler intrinsics for all the common
bit
> operations will be helpful.  I suspect other parts of the GC are performance
> limited by bitmap scanning.

There is no other bitmap scanning in GC AFAIR.

https://codereview.appspot.com/9223046/diff/75001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/75001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:201: roundup(int32 x, int32 n)
use ROUND macro

[DMorsing, 2013-05-23 06:25:09]

https://codereview.appspot.com/9223046/diff/62001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/cmd/gc/pgen.c#newcode309
src/cmd/gc/pgen.c:309: if(thistype != nil)
On 2013/05/22 23:19:11, cshapiro1 wrote:
> I started by using the fn->dcl list but I had difficulty getting complete
> information in all cases.  I believe the issue was with retrieving the this
> pointer type but I might be misremembering.
> 

The this parameter shows up in dcl as a node with class PPARAM. The initial
offset of the parameters should be in their xoffset field.

You're doing a lot of extra work trying to figure out how the arguments are
arranged, when we already have this information in the AST.

[cshapiro1, 2013-05-23 21:56:10]

On 2013/05/23 06:25:09, DMorsing wrote:
> The this parameter shows up in dcl as a node with class PPARAM. The initial
> offset of the parameters should be in their xoffset field.

I believe this statement to not generally be true.

Consider the following function definition 

  func (*TypeAssertionError) RuntimeError() {}

If I follow your suggestion I will mistake this function as not having a
receiver argument as curfn->dcl will be NULL.  This might not be the only issue
but it was the first I encountered.

> You're doing a lot of extra work trying to figure out how the arguments are
> arranged, when we already have this information in the AST.

I am not yet convinced there is a convenient and reliable way to get this from
the AST.  I am happy to be proven wrong.

As I mentioned up-thread, my next change will be computing this information
somewhat differently so this code (which I agree is not the most elegant) will
be relatively short lived.

[cshapiro1, 2013-05-23 22:14:54]

PTAL

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1397: // locals need to be scanned.
On 2013/05/23 05:28:58, dvyukov wrote:
> You "Scan the entire stack frame" and then you additionally scan args in
> "if(f->args > 0) {".
> What is true?
> 1. The comment is incorrect.
> 2. The code under "if" is not supposed to be executed in this case.
> 3. Args is not a part of the entire stack frame.
> 4. I am missing something.

Item number 3 is true.  Arguments are located at addresses above the virtual FP
which is within the stack frame of the caller, not the stack frame currently
being scanned.

The callee has knowledge of what arguments are live in the frame of the caller. 
This is why we scan part of the caller frame in the callee.  The doframe signal
is used when the callee does not have knowledge of the argument size.  In that
case the caller scans the arguments area within its own frame.

If you think some of this information is not correctly described by my comment,
I am happy to rewrite it to emphasize any subtlties.  Just let me know what
might be missing.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1425: for((j = (rem < 32) ? rem : 32); j > 0; j--) {
On 2013/05/23 05:28:58, dvyukov wrote:
> There is no other bitmap scanning in GC AFAIR.

That should have read "I suspect other parts of the GC are performance
*will be* limited by bitmap scanning."  At the moment, I believe you are right.

https://codereview.appspot.com/9223046/diff/75001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/75001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:201: roundup(int32 x, int32 n)
Done.

There are too many ways to round a number in this system.

[dvyukov, 2013-05-24 04:42:07]

LGTM on the runtime part

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1397: // locals need to be scanned.
On 2013/05/23 22:14:55, cshapiro1 wrote:
> On 2013/05/23 05:28:58, dvyukov wrote:
> > You "Scan the entire stack frame" and then you additionally scan args in
> > "if(f->args > 0) {".
> > What is true?
> > 1. The comment is incorrect.
> > 2. The code under "if" is not supposed to be executed in this case.
> > 3. Args is not a part of the entire stack frame.
> > 4. I am missing something.
> 
> Item number 3 is true.  Arguments are located at addresses above the virtual
FP
> which is within the stack frame of the caller, not the stack frame currently
> being scanned.
> 
> The callee has knowledge of what arguments are live in the frame of the
caller. 
> This is why we scan part of the caller frame in the callee.  The doframe
signal
> is used when the callee does not have knowledge of the argument size.  In that
> case the caller scans the arguments area within its own frame.
> 
> If you think some of this information is not correctly described by my
comment,
> I am happy to rewrite it to emphasize any subtlties.  Just let me know what
> might be missing.

I think now I am starting to understand. Thanks!
Yes, please add the first 2 paragraphs of your answer to the comment.

[DMorsing, 2013-05-24 06:15:45]

On 2013/05/23 21:56:10, cshapiro1 wrote:
> On 2013/05/23 06:25:09, DMorsing wrote:
> > The this parameter shows up in dcl as a node with class PPARAM. The initial
> > offset of the parameters should be in their xoffset field.
> 
> I believe this statement to not generally be true.
> 
> Consider the following function definition 
> 
>   func (*TypeAssertionError) RuntimeError() {}
> 
> If I follow your suggestion I will mistake this function as not having a
> receiver argument as curfn->dcl will be NULL.  This might not be the only
issue
> but it was the first I encountered.
> 
> > You're doing a lot of extra work trying to figure out how the arguments are
> > arranged, when we already have this information in the AST.
> 
> I am not yet convinced there is a convenient and reliable way to get this from
> the AST.  I am happy to be proven wrong.
> 
> As I mentioned up-thread, my next change will be computing this information
> somewhat differently so this code (which I agree is not the most elegant) will
> be relatively short lived.

You're right, Sorry for being so pushy.

In any case, you can get the width of the function parameters via
fn->type->argwid and you can get an iterator that walks the receiver and
arguments via funcfirst() and funcnext().

[cshapiro1, 2013-05-28 21:07:55]

On 2013/05/24 06:15:45, DMorsing wrote:
> You're right, Sorry for being so pushy.

No problem.  Not having the receiver in curfn->dcl seems like an oversight.  It
might be something worth fixing.

> In any case, you can get the width of the function parameters via
> fn->type->argwid and you can get an iterator that walks the receiver and
> arguments via funcfirst() and funcnext().

Thanks, fn->type->argwid is a significant improvement.  It is in my most recent
upload. 

The funcfirst and funcnext calls would still leave me walking the out arguments
by the means already in place.  I should be able to switch to using it once I
treat the out arguments like local variables.  As an aside, I think the
internals of walktype might look a little better if it used structfirst and
structnext.  I plan to clean that up in subsequent change.

[cshapiro1, 2013-05-28 21:08:11]

PTAL

[iant, 2013-05-28 21:23:35]

LGTM

Let's get this in.  Thanks to you and to all the reviewers.

https://codereview.appspot.com/9223046/diff/88001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/88001/src/cmd/gc/pgen.c#newcode273
src/cmd/gc/pgen.c:273: xoffset = 0;
I think we should have a comment somewhere explaining why this xoffset = 0 is,
in a sense, not used.  That is, why calling walktype multiple times in
pointermap works OK.

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1418: // Scan the arguments area.
Should we have a check here that f->ptrs.len is large enough to cover the entire
argument area?

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:248:
((uint32*)func[nfunc-1].ptrs.array)[func[nfunc-1].ptrs.len++] = sym->value;
Add a check that .ptrs.len < .ptrs.cap?

[cshapiro1, 2013-05-29 00:29:19]

I addressed the remaining review comments in my last upload.  I will submit
after doing a final sync and test.

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/62001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1397: // locals need to be scanned.
I have improved the comment.  Let me know what you think.

https://codereview.appspot.com/9223046/diff/88001/src/cmd/gc/pgen.c
File src/cmd/gc/pgen.c (right):

https://codereview.appspot.com/9223046/diff/88001/src/cmd/gc/pgen.c#newcode273
src/cmd/gc/pgen.c:273: xoffset = 0;
Done.

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/mgc0.c
File src/pkg/runtime/mgc0.c (right):

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/mgc0.c#newc...
src/pkg/runtime/mgc0.c:1418: // Scan the arguments area.
These values are produced by the symbol table construction.  I have a check in
the symbol table construction to ensure the pointer map is not too large.

I would like to start dropping long runs of trailing 0s at the end of the
pointer map so this check would be short lived.  However, I can probably do a
better job checking this sort of stuf once I resolve issues in the compiler
regarding nosplits, varargs, and other things that confused the argument size
tracking.

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/symtab.c
File src/pkg/runtime/symtab.c (right):

https://codereview.appspot.com/9223046/diff/88001/src/pkg/runtime/symtab.c#ne...
src/pkg/runtime/symtab.c:248:
((uint32*)func[nfunc-1].ptrs.array)[func[nfunc-1].ptrs.len++] = sym->value;
Done.

[cshapiro, 2013-05-29 00:54:19]

*** Abandoned ***

[cshapiro1, 2013-05-29 00:59:16]

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

cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of argument
pointer locations

With this change the compiler emits a bitmap for each function
covering its stack frame arguments area.  If an argument word
is known to contain a pointer, a bit is set.  The garbage
collector reads this information when scanning the stack by
frames and uses it to ignores locations known to not contain a
pointer.

R=golang-dev, bradfitz, daniel.morsing, dvyukov, khr, khr, iant, cshapiro
CC=golang-dev
https://codereview.appspot.com/9223046

[albert.strasheim, 2013-06-03 09:45:12]

On 2013/05/29 00:59:16, cshapiro1 wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=02e5cb24c95a ***
> cmd/5l, cmd/6l, cmd/8l, cmd/gc, runtime: generate and use bitmaps of argument
> pointer locations

I've run into some fun with this change...

https://code.google.com/p/go/issues/detail?id=5621

[cshapiro, 2013-08-03 00:29:22]

*** Abandoned ***

[cshapiro, 2013-08-03 00:29:31]

*** Abandoned ***