Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(292)

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

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 10 months ago by cshapiro1
Modified:
10 years, 8 months ago
CC:
golang-dev, bradfitz, DMorsing, dvyukov, khr, khr1, iant
Visibility:
Public.

Description

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.

Patch Set 1 #

Patch Set 2 : diff -r bbe324079abe https://code.google.com/p/go/ #

Patch Set 3 : diff -r bbe324079abe https://code.google.com/p/go/ #

Patch Set 4 : diff -r bbe324079abe https://code.google.com/p/go/ #

Total comments: 8

Patch Set 5 : diff -r bbe324079abe https://code.google.com/p/go/ #

Patch Set 6 : diff -r bbe324079abe https://code.google.com/p/go/ #

Total comments: 6

Patch Set 7 : diff -r bbe324079abe https://code.google.com/p/go/ #

Patch Set 8 : diff -r bbe324079abe https://code.google.com/p/go/ #

Patch Set 9 : diff -r bbe324079abe https://code.google.com/p/go/ #

Total comments: 15

Patch Set 10 : diff -r 3623b5f14f72 https://code.google.com/p/go/ #

Patch Set 11 : diff -r 3623b5f14f72 https://code.google.com/p/go/ #

Total comments: 38

Patch Set 12 : diff -r 8e41e8e4a2ab https://code.google.com/p/go/ #

Total comments: 21

Patch Set 13 : diff -r 8e41e8e4a2ab https://code.google.com/p/go/ #

Patch Set 14 : diff -r 8e41e8e4a2ab https://code.google.com/p/go/ #

Total comments: 2

Patch Set 15 : diff -r cc83df684054 https://code.google.com/p/go/ #

Patch Set 16 : diff -r cc83df684054 https://code.google.com/p/go/ #

Total comments: 6

Patch Set 17 : diff -r 81ccdb178fd7 https://code.google.com/p/go/ #

Patch Set 18 : diff -r 81ccdb178fd7 https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+454 lines, -13 lines) Patch
M src/cmd/5g/peep.c View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +4 lines, -0 lines 0 comments Download
M src/cmd/5g/reg.c View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/5l/5.out.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/5l/l.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/5l/obj.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +33 lines, -0 lines 0 comments Download
M src/cmd/6g/peep.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/6g/reg.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/6l/6.out.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/6l/l.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/6l/obj.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +33 lines, -0 lines 0 comments Download
M src/cmd/6l/optab.c View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/8g/peep.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/8g/reg.c View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/8l/8.out.h View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/8l/l.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +2 lines, -0 lines 0 comments Download
M src/cmd/8l/obj.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +33 lines, -0 lines 0 comments Download
M src/cmd/8l/optab.c View 1 1 chunk +2 lines, -0 lines 0 comments Download
A src/cmd/gc/bv.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 1 chunk +95 lines, -0 lines 0 comments Download
M src/cmd/gc/go.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 3 chunks +17 lines, -0 lines 0 comments Download
M src/cmd/gc/pgen.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 3 chunks +146 lines, -0 lines 0 comments Download
M src/cmd/ld/lib.c View 1 2 3 4 5 6 7 8 9 10 11 12 2 chunks +7 lines, -1 line 0 comments Download
M src/pkg/runtime/extern.go View 1 2 3 4 5 6 7 8 9 10 11 12 13 1 chunk +4 lines, -3 lines 0 comments Download
M src/pkg/runtime/mgc0.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +35 lines, -7 lines 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +2 lines, -0 lines 0 comments Download
M src/pkg/runtime/symtab.c View 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 2 chunks +19 lines, -2 lines 0 comments Download

Messages

Total messages: 40
cshapiro1
Hello golang-dev@googlegroups.com, I'd like you to review this change to https://code.google.com/p/go/
10 years, 10 months ago (2013-05-08 02:22:10 UTC) #1
bradfitz
Impressive code/test ratio! Update Issue nnnn? Does this make anything better which you can demonstrate ...
10 years, 10 months ago (2013-05-08 03:35:06 UTC) #2
cshapiro1
I will update the change description with the relevant issue. This infrastructure is currently hidden ...
10 years, 10 months ago (2013-05-08 05:08:44 UTC) #3
bradfitz
Cool, thanks for the background. Putting some of that in the issue and/or CL description ...
10 years, 10 months ago (2013-05-08 05:12:01 UTC) #4
cshapiro1
Will do. Thanks. On Tue, May 7, 2013 at 10:11 PM, Brad Fitzpatrick <bradfitz@golang.org>wrote: > ...
10 years, 10 months ago (2013-05-08 05:28:23 UTC) #5
DMorsing
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, ...
10 years, 10 months ago (2013-05-08 09:04:55 UTC) #6
dvyukov
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#newcode239 src/pkg/runtime/symtab.c:239: fgc[nfunc-1] = runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1); is it meant ...
10 years, 10 months ago (2013-05-08 18:14:25 UTC) #7
cshapiro1
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 ...
10 years, 10 months ago (2013-05-08 18:25:16 UTC) #8
cshapiro1
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#newcode239 src/pkg/runtime/symtab.c:239: fgc[nfunc-1] = runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1); Yes, that is ...
10 years, 10 months ago (2013-05-08 18:30:30 UTC) #9
dvyukov
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#newcode239 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, ...
10 years, 10 months ago (2013-05-09 09:18:05 UTC) #10
cshapiro1
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#newcode239 src/pkg/runtime/symtab.c:239: fgc[nfunc-1] = runtime·mallocgc(sym->value*sizeof(*fgc), FlagNoPointers, 0, 1); Thanks. Done.
10 years, 10 months ago (2013-05-09 21:02:49 UTC) #11
khr
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, ...
10 years, 10 months ago (2013-05-09 21:49:53 UTC) #12
cshapiro1
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 ...
10 years, 10 months ago (2013-05-09 22:00:39 UTC) #13
khr1
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://codereview.appspot.com/9223046/diff/22001/src/cmd/gc/go.h> > File ...
10 years, 10 months ago (2013-05-09 22:26:37 UTC) #14
cshapiro1
On Thu, May 9, 2013 at 3:26 PM, Keith Randall <khr@google.com> wrote: > That's better. ...
10 years, 10 months ago (2013-05-09 22:35:54 UTC) #15
cshapiro1
On 2013/05/09 22:26:37, khr1 wrote: > Check it at runtime and runtime·throw if it is ...
10 years, 10 months ago (2013-05-10 00:52:08 UTC) #16
cshapiro1
Ping?
10 years, 10 months ago (2013-05-20 20:39:32 UTC) #17
dvyukov
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#newcode813 src/pkg/runtime/runtime.h:813: #pragma varargck type "a" Slice please send this is ...
10 years, 10 months ago (2013-05-21 06:55:55 UTC) #18
dvyukov
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#newcode1405 src/pkg/runtime/mgc0.c:1405: if(f->locals == 0 || *(bool*)doframe == true) add a ...
10 years, 10 months ago (2013-05-21 07:21:40 UTC) #19
cshapiro1
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#newcode1405 src/pkg/runtime/mgc0.c:1405: if(f->locals == 0 || *(bool*)doframe == true) This ...
10 years, 10 months ago (2013-05-21 23:08:56 UTC) #20
iant
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 ...
10 years, 10 months ago (2013-05-22 00:41:00 UTC) #21
cshapiro1
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 ...
10 years, 10 months ago (2013-05-22 06:35:21 UTC) #22
dvyukov
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#newcode1405 src/pkg/runtime/mgc0.c:1405: if(f->locals == 0 || *(bool*)doframe == true) On 2013/05/21 ...
10 years, 10 months ago (2013-05-22 07:07:13 UTC) #23
DMorsing
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 ...
10 years, 10 months ago (2013-05-22 09:29:47 UTC) #24
cshapiro1
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): ...
10 years, 10 months ago (2013-05-22 23:19:10 UTC) #25
dvyukov
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#newcode1397 src/pkg/runtime/mgc0.c:1397: // locals need to be scanned. On 2013/05/22 23:19:11, ...
10 years, 10 months ago (2013-05-23 05:28:58 UTC) #26
DMorsing
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: > ...
10 years, 10 months ago (2013-05-23 06:25:09 UTC) #27
cshapiro1
On 2013/05/23 06:25:09, DMorsing wrote: > The this parameter shows up in dcl as a ...
10 years, 10 months ago (2013-05-23 21:56:10 UTC) #28
cshapiro1
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#newcode1397 src/pkg/runtime/mgc0.c:1397: // locals need to be scanned. On 2013/05/23 ...
10 years, 10 months ago (2013-05-23 22:14:54 UTC) #29
dvyukov
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#newcode1397 src/pkg/runtime/mgc0.c:1397: // locals need to ...
10 years, 10 months ago (2013-05-24 04:42:07 UTC) #30
DMorsing
On 2013/05/23 21:56:10, cshapiro1 wrote: > On 2013/05/23 06:25:09, DMorsing wrote: > > The this ...
10 years, 10 months ago (2013-05-24 06:15:45 UTC) #31
cshapiro1
On 2013/05/24 06:15:45, DMorsing wrote: > You're right, Sorry for being so pushy. No problem. ...
10 years, 10 months ago (2013-05-28 21:07:55 UTC) #32
cshapiro1
PTAL
10 years, 10 months ago (2013-05-28 21:08:11 UTC) #33
iant
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 ...
10 years, 10 months ago (2013-05-28 21:23:35 UTC) #34
cshapiro1
I addressed the remaining review comments in my last upload. I will submit after doing ...
10 years, 10 months ago (2013-05-29 00:29:19 UTC) #35
cshapiro
*** Abandoned ***
10 years, 10 months ago (2013-05-29 00:54:19 UTC) #36
cshapiro1
*** 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 ...
10 years, 10 months ago (2013-05-29 00:59:16 UTC) #37
albert.strasheim
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, ...
10 years, 10 months ago (2013-06-03 09:45:12 UTC) #38
cshapiro
*** Abandoned ***
10 years, 8 months ago (2013-08-03 00:29:22 UTC) #39
cshapiro
10 years, 8 months ago (2013-08-03 00:29:31 UTC) #40
Message was sent while issue was closed.
*** Abandoned ***
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b