code review 11683043: cc: generate argument pointer maps for C functions.

cc: generate argument pointer maps for C functions.

[khr, 2013-07-23 22:13:36]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://khr%40golang.org@code.google.com/p/go/

[rsc, 2013-07-24 04:45:59]

LGTM

https://codereview.appspot.com/11683043/diff/17001/src/cmd/cc/pgen.c
File src/cmd/cc/pgen.c (right):

https://codereview.appspot.com/11683043/diff/17001/src/cmd/cc/pgen.c#newcode645
src/cmd/cc/pgen.c:645: static int32 pointermap_type(Type *t, int32 offset, int32
baseidx)
line break after int32, sorry

https://codereview.appspot.com/11683043/diff/17001/src/cmd/cc/pgen.c#newcode694
src/cmd/cc/pgen.c:694: // Compute a bit vector to describe the pointer
containing locations
blank line above, sorry

https://codereview.appspot.com/11683043/diff/17001/src/cmd/cc/pgen.c#newcode729
src/cmd/cc/pgen.c:729: if(t->etype == TVOID) continue;
break line

[rsc, 2013-07-24 04:47:25]

FWIW, I would have just allocated the bitmap instead of the multipass. The
bitmap is smaller than the instructions being generated, and it would give
an opportunity to trim nptrs if the function ends with lots of
non-pointers. However, these aren't going to matter much in the C we have
in the runtime. (I guess packages can have C too, though.) Anyway, it's
fine as is too.

[khr, 2013-07-24 16:41:11]

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

cc: generate argument pointer maps for C functions.

R=golang-dev, rsc
CC=golang-dev
https://codereview.appspot.com/11683043

[dave_cheney.net, 2013-07-24 21:53:08]

On 2013/07/24 16:41:11, khr wrote:
> *** Submitted as https://code.google.com/p/go/source/detail?r=bb75d03e6ccb ***
> 
> cc: generate argument pointer maps for C functions.
> 
> R=golang-dev, rsc
> CC=golang-dev
> https://codereview.appspot.com/11683043

This change has broken all the arm builders. Would you please revert it
temporarily.

[khr1, 2013-07-24 21:54:02]

Sure, sorry.


On Wed, Jul 24, 2013 at 2:53 PM, <dave@cheney.net> wrote:

> On 2013/07/24 16:41:11, khr wrote:
>
>> *** Submitted as
>>
>
https://code.google.com/p/go/**source/detail?r=bb75d03e6ccb<https://code.goog...
>
>  cc: generate argument pointer maps for C functions.
>>
>
>  R=golang-dev, rsc
>> CC=golang-dev
>>
https://codereview.appspot.**com/11683043<https://codereview.appspot.com/1168...
>>
>
> This change has broken all the arm builders. Would you please revert it
> temporarily.
>
>
https://codereview.appspot.**com/11683043/<https://codereview.appspot.com/116...
>

[dave_cheney.net, 2013-07-24 21:56:56]

Thanks Keith.

On Thu, Jul 25, 2013 at 7:54 AM, Keith Randall <khr@google.com> wrote:
> Sure, sorry.
>
>
> On Wed, Jul 24, 2013 at 2:53 PM, <dave@cheney.net> wrote:
>>
>> On 2013/07/24 16:41:11, khr wrote:
>>>
>>> *** Submitted as
>>
>> https://code.google.com/p/go/source/detail?r=bb75d03e6ccb ***
>>
>>> cc: generate argument pointer maps for C functions.
>>
>>
>>> R=golang-dev, rsc
>>> CC=golang-dev
>>> https://codereview.appspot.com/11683043
>>
>>
>> This change has broken all the arm builders. Would you please revert it
>> temporarily.
>>
>> https://codereview.appspot.com/11683043/
>
>

[binet, 2013-07-25 09:25:34]

just a small typo

https://codereview.appspot.com/11683043/diff/23001/src/cmd/cc/pgen.c
File src/cmd/cc/pgen.c (right):

https://codereview.appspot.com/11683043/diff/23001/src/cmd/cc/pgen.c#newcode642
src/cmd/cc/pgen.c:642: // Makes a bitmap marking the the pointers in t.  t
starts at the given byte
- Makes a bitmap marking the the pointers
+ Makes a bitmap marking the pointers

[rsc, 2013-07-25 13:40:22]

Aha, I knew there was a reason I put the FUNCDATA at the bottom of the
function. It was that if you put it at the top you get inscrutable errors
from 5l.

What was the reason you moved it to the top?

Russ

[khr1, 2013-07-25 19:28:35]

At the bottom it often gets lost somehow.  I'm not sure why, maybe without
control flow to it is gets elided?  I'll investigate some more...


On Thu, Jul 25, 2013 at 6:40 AM, Russ Cox <rsc@golang.org> wrote:

> Aha, I knew there was a reason I put the FUNCDATA at the bottom of the
> function. It was that if you put it at the top you get inscrutable errors
> from 5l.
>
> What was the reason you moved it to the top?
>
> Russ
>

[rsc, 2013-07-25 20:07:20]

Tricky. I agree that it _should_ be at the top, fwiw.

Russ

[DMorsing, 2013-07-25 22:09:15]

On Thu, Jul 25, 2013 at 9:28 PM, Keith Randall <khr@google.com> wrote:
> At the bottom it often gets lost somehow.  I'm not sure why, maybe without
> control flow to it is gets elided?  I'll investigate some more...
>

Could very well be. The register optimizer doesn't know about metadata
instructions and will remove dead branches. Only reason it worked
previously was because we inserted metadata instructions at the entry
point.

Some things got were missed when the new symbol table instructions
were added. We skip the TYPE instruction in the peephole optimizer and
regopt, but not FUNCDATA and PCDATA.