code review 6530047: cmd/5a, cmd/6a, cmd/8a: introduce TYPE instruction

cmd/5a, cmd/6a, cmd/8a: introduce TYPE instruction

See src/cmd/6a/doc.go for details.

Convert package bytes as a demo.
There is more assembly to convert.

[rsc, 2012-09-19 04:42:27]

Hello golang-dev@googlegroups.com,

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

[dfc, 2012-09-19 06:52:35]

On 2012/09/19 04:42:27, rsc wrote:
> Hello mailto:golang-dev@googlegroups.com,
> 
> I'd like you to review this change to
> https://code.google.com/p/go/

on linux/amd64 the build fails, i'm not sure if this is intentional or not, as
you said this was a proof of concept.


pkg/bytes
/home/dfc/go/src/pkg/bytes/asm_amd64.s:94 use of old syntax to refer to argument
frame
go tool dist: FAILED: /home/dfc/go/pkg/tool/linux_amd64/6a -I $WORK -DGOOS_linux
-DGOARCH_amd64 -o $WORK/asm_amd64.o /home/dfc/go/src/pkg/bytes/asm_amd64.s

[dfc, 2012-09-19 07:03:07]

On 2012/09/19 06:52:35, dfc wrote:
> On 2012/09/19 04:42:27, rsc wrote:
> > Hello mailto:golang-dev@googlegroups.com,
> > 
> > I'd like you to review this change to
> > https://code.google.com/p/go/
> 
> on linux/amd64 the build fails, i'm not sure if this is intentional or not, as
> you said this was a proof of concept.
> 
> 
> pkg/bytes
> /home/dfc/go/src/pkg/bytes/asm_amd64.s:94 use of old syntax to refer to
argument
> frame
> go tool dist: FAILED: /home/dfc/go/pkg/tool/linux_amd64/6a -I $WORK
-DGOOS_linux
> -DGOARCH_amd64 -o $WORK/asm_amd64.o /home/dfc/go/src/pkg/bytes/asm_amd64.s

same on linux/arm.


pkg/bytes
/net/10.42.0.1/export/pi/home/dfc/go/src/pkg/bytes/asm_arm.s:30 use of old
syntax to refer to argument frame
go tool dist: FAILED: /net/10.42.0.1/export/pi/home/dfc/go/pkg/tool/linux_arm/5a
-I $WORK -DGOOS_linux -DGOARCH_arm -o $WORK/asm_arm.o
/net/10.42.0.1/export/pi/home/dfc/go/src/pkg/bytes/asm_arm.s

[dfc, 2012-09-19 07:03:18]

http://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody
File src/cmd/cc/gotypebody (right):

http://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcode37
src/cmd/cc/gotypebody:37: "[]",		PtrSize+2*4,	PtrSize,	GoSlice,
No support for arrays ?

[iant, 2012-09-19 14:45:36]

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody
File src/cmd/cc/gotypebody (right):

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcode31
src/cmd/cc/gotypebody:31: } gotypes[] = {
I don't see pointers in this list.

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcode32
src/cmd/cc/gotypebody:32: "<-chan ",	PtrSize,	PtrSize,	GoBasic,
Why not just chan?  I don't see how readtype will ever return "<-chan".

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcode35
src/cmd/cc/gotypebody:35: "map[",		PtrSize,	PtrSize,	GoBasic,
Looking at readtype, I don't see how you will ever get a type that matches
"map[".

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcode40
src/cmd/cc/gotypebody:40: "int",		4,		4,		GoBasic,
Do you want to use IntSize here, or just remember to change it when the time
comes?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcod...
src/cmd/cc/gotypebody:128: gen->opsize = 4;
IntSize?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcod...
src/cmd/cc/gotypebody:309: readtype(char **sp, char **typp)
Why not just have readtype always canonicalize [] and *?  Then you can simplify
gomatch.

[nigeltao, 2012-09-20 00:38:37]

I can't view some of the files in codereview:

https://codereview.appspot.com/6530047/diff/5001/src/cmd/5a/y.tab.c gives me
"Can't parse the patch to chunks".

https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/y.tab.c and
https://codereview.appspot.com/6530047/diff/5001/src/cmd/8a/y.tab.c give me "Bad
content. Try to upload again."

Borked for all or just me?

[iant2, 2012-09-20 00:50:37]

On Wed, Sep 19, 2012 at 5:38 PM,  <nigeltao@golang.org> wrote:
> I can't view some of the files in codereview:
>
> https://codereview.appspot.com/6530047/diff/5001/src/cmd/5a/y.tab.c
> gives me "Can't parse the patch to chunks".
>
> https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/y.tab.c and
> https://codereview.appspot.com/6530047/diff/5001/src/cmd/8a/y.tab.c give
> me "Bad content. Try to upload again."
>
> Borked for all or just me?

Doesn't work for me either, but it doesn't matter, as those are generated
files.

Ian

[nigeltao, 2012-09-20 01:51:06]

https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/lex.c
File src/cmd/6a/lex.c (right):

https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/lex.c#newcode174
src/cmd/6a/lex.c:174: #define CVT(x, y) ((x)<<8 | (y))
s/x, y/f, t/ to match 6a and 8a?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/lex.c#newcode432
src/cmd/6a/lex.c:432: "JB",		LTYPER,	AJCS, 1,	/* alternate */
Does this have an opsize of 1 just because the name ends in "B"?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/lex.c#newcode720
src/cmd/6a/lex.c:720: "FMOVB",	LTYPE3, AFMOVB, 1,
Is 6a's FMOV what other x86 assemblers call FLD/FST?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/lex.c#newcode1217
src/cmd/6a/lex.c:1217: yyerror("%s expects src size=%d but have src size=%d",
anames[a], tosize, g2->to.opsize);
s/src/dst/g and similarly for 5a and 8a.

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody
File src/cmd/cc/gotypebody (right):

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcod...
src/cmd/cc/gotypebody:150: s = strdup(skip(s));
Out of curiousity, is the strdup unnecessary? Just a defensive habit?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcod...
src/cmd/cc/gotypebody:283: return '0' <= c && c <= '9' || 'a' <= c && c <= 'z'
|| 'A' <= c && c <= 'Z' || c >= 0x80;
Underscore?

https://codereview.appspot.com/6530047/diff/5001/src/cmd/cc/gotypebody#newcod...
src/cmd/cc/gotypebody:323: yyerror("syntax error in TYPE: missing open [ in map
type");
Isn't "map" a shorthand for "map[T]U"?

[r, 2012-09-20 06:17:15]

The spec (to use a generous term) for this feature is vague. For an assembler
already abysmally documented, this isn't helping.

Also I worry about where this will lead. It marks a profound change in direction
for the assembler, moving it from a pure text->.6 converter into something with
pretty deep knowledge about Go.

I'm not saying "no", I'm just clearing my throat.

[rsc, 2012-09-20 19:30:30]

On Thu, Sep 20, 2012 at 2:17 AM,  <r@golang.org> wrote:
> The spec (to use a generous term) for this feature is vague. For an
> assembler already abysmally documented, this isn't helping.

Are there specific things you'd like to me add or questions you'd like
me to answer in
http://codereview.appspot.com/6530047/diff/5001/src/cmd/6a/doc.go?

> Also I worry about where this will lead. It marks a profound change in
> direction for the assembler, moving it from a pure text->.6 converter
> into something with pretty deep knowledge about Go.

It does, yes. Maybe not pretty deep but definitely knowledge.

Russ

[nigeltao, 2012-09-20 23:31:31]

On 21 September 2012 05:30, Russ Cox <rsc@golang.org> wrote:
>> Also I worry about where this will lead. It marks a profound change in
>> direction for the assembler, moving it from a pure text->.6 converter
>> into something with pretty deep knowledge about Go.
>
> It does, yes. Maybe not pretty deep but definitely knowledge.

Thinking out loud, could you write the func as a comment that a
vet-like tool outside of 5a/6a/8a could check? You'd have to manually
update the offsets (although the tool could be fix-like in updating
them), but you have to do some manual work anyway since we have MOVL
and MOVQ but not MOV.

// TYPE "func(x []byte, b byte) (ret int)"
TEXT ·IndexByte(SB),7,$0
        MOVQ x_ptr+0(FP), SI
        MOVL x_len+8(FP), BX
        MOVB b+16(FP), AL
        MOVQ SI, DI
        // etc

[bradfitzgoog, 2012-09-20 23:44:18]

On Thu, Sep 20, 2012 at 4:31 PM, Nigel Tao <nigeltao@golang.org> wrote:

> On 21 September 2012 05:30, Russ Cox <rsc@golang.org> wrote:
> >> Also I worry about where this will lead. It marks a profound change in
> >> direction for the assembler, moving it from a pure text->.6 converter
> >> into something with pretty deep knowledge about Go.
> >
> > It does, yes. Maybe not pretty deep but definitely knowledge.
>
> Thinking out loud, could you write the func as a comment that a
> vet-like tool outside of 5a/6a/8a could check?
>

Do [568]a have aspirations of being (or remaining) generic linkers for
multiple languages?

I thought they were already forked to be Go's.

[rsc, 2012-09-21 04:13:30]

Never mind, I will try something else.

[rsc, 2012-09-21 04:16:08]

*** Abandoned ***

[rsc, 2012-09-21 04:39:04]

> Thinking out loud, could you write the func as a comment that a
> vet-like tool outside of 5a/6a/8a could check? You'd have to manually
> update the offsets (although the tool could be fix-like in updating
> them), but you have to do some manual work anyway since we have MOVL
> and MOVQ but not MOV.

The int change is not that hard and requires editing source files by
hand regardless of what we do here. I was trying to avoid assembly
changes for pure alignment changes (two are planned). Instead I have
written a tool to check the assembly against the Go declarations. When
it comes time to do the alignment changes I will make the program
apply the updates. I don't expect this to be a general tool we
advertise.

Russ