[dev.power64] code review 123170043: cmd/9g: first check point

cmd/9g: first check point

[minux, 2014-08-11 07:33:58]

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

I'd like you to review this change to the dev.power64 branch of
https://code.google.com/p/go

[rsc, 2014-08-11 17:55:39]

These are copied from 6g, right? Can you do an hg cp -A so that we can see
diffs?

[minux, 2014-08-11 18:10:48]

On 2014/08/11 17:55:39, rsc wrote:
> These are copied from 6g, right? Can you do an hg cp -A so that we can see
> diffs?
Done. PTAL.

Please disregard the copyright headers for now. Thanks.

[rsc, 2014-08-11 19:00:38]

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c
File src/cmd/9g/cgen.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode160
src/cmd/9g/cgen.c:160: if(n->addable && res->addable) {
res->addable should be true. there is code above that does if(!res->addable) {
... goto ret; }
so this should be just

if(n->addable) {
...

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode161
src/cmd/9g/cgen.c:161: if(n->op == OREGISTER || res->op == OREGISTER /*||
please remove /* ... */
if(n->op == OREGISTER || res->op == OREGISTER)
    gmove(n, res);
else {

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode204
src/cmd/9g/cgen.c:204: // TODO(minux): we shouldn't re-order FP comparisons, but
then we need to synthesize
I don't know what you mean by re-order. I think the code in 6g already does the
right thing. I think this can be deleted unless I'm missing something.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode539
src/cmd/9g/cgen.c:539: case OMUL:
left shift?

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode656
src/cmd/9g/cgen.c:656: } else
put else if on one line. i know it's not always done, but it will make this
clearer.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode709
src/cmd/9g/cgen.c:709: nodconst(&n2, types[tptr], v*w);
seems like you should keep the if(v*w != 0)

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode731
src/cmd/9g/cgen.c:731: if(nl->type->bound < 32767)
< (1<<15)-1

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode755
src/cmd/9g/cgen.c:755: } else
} else if

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode766
src/cmd/9g/cgen.c:766: }/* else if(w == 1 || w == 2 || w == 4 || w == 8) {
If we know we have to use OADD, then I think we can at least do:

} else {
    if(w == 1) {
        /* w already scaled */
    } /* else if(w == 2 || w == 4 || w == 8) {
        // TODO(minux): scale using shift
    } */ else {
        ... OMUL ...
    }
    gins(optoas(OADD, types[tptr]), &n2, &n3);
}

That will do the right thing for byte slices and strings, which don't need
shifts.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode818
src/cmd/9g/cgen.c:818: gins(AMOVD, &n1, &n2);
Pretty sure this is not doing an LEAQ equivalent here.
n1 is tempname, which is a direct reference (name(SB) not $name(SB)).
Need to add the $ by editing p->from.type I think.
Or by using an OADDR node like below.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode845
src/cmd/9g/cgen.c:845: // XXX: 5g has this: Release res so that it is available
for cgen_call. Pick it up again after the call.
s/XXX/TODO(minux)/

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode898
src/cmd/9g/cgen.c:898: //ginscon(optoas(OADD, types[tptr]), n->xoffset, res);
It sounds like you should write ginscon instead of duplicating this code.
ginscon will be a good place to decide about the use of immediate constants.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1023: a->xoffset = fp->width + 8; // +8: saved lr at 0(SP)
+ widthptr please

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1267: // and cmpli accepts 16-bit unsigned immediate.
maybe the TODO is to use ginscon.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1282: /*if(isfloat[nr->type->etype] && (n->op == OEQ || n->op
== ONE)) {
Delete if not needed.
Add TODO if needed.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1352: return t->width + 8;	// correct for LR
widthptr please

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gg.h
File src/cmd/9g/gg.h (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gg.h#newcode12
src/cmd/9g/gg.h:12: #define noimpl sysfatal("%s not implemented (%s:%d).",
__func__, __FILE__, __LINE__)
// TODO(minux): Remove when no longer used.

This #define is okay here but cannot land in the main tree.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gg.h#newcode17
src/cmd/9g/gg.h:17: EXTERN	uchar	reg[NREG+NREG];
Is this really NREG+NFREG?

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gg.h#newcode29
src/cmd/9g/gg.h:29: //extern	int	addptr;
Delete dead code. Don't comment it out.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c
File src/cmd/9g/ggen.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode146
src/cmd/9g/ggen.c:146: ginsBL(Node *reg, Node *f) {
\n{

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode319
src/cmd/9g/ggen.c:319: nodsp.xoffset = 8;
widthptr

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode428
src/cmd/9g/ggen.c:428: nod.xoffset = fp->width + 8; // +8: saved LR at 0(R1)
widthptr

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode458
src/cmd/9g/ggen.c:458: nod1.xoffset = fp->width + 8; // +8: saved lr at 0(SP)
widthptr

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode598
src/cmd/9g/ggen.c:598: // A%B = A-(A/B*B)
The power div doesn't give you mod for free? I'm a little surprised.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode618
src/cmd/9g/ggen.c:618: USED(n);
fatal("cgen_asop"); // no longer used

And move back up above, to line up with deletion in 6g.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode766
src/cmd/9g/ggen.c:766: regalloc(&n1, t, res);
use cgenr.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode768
src/cmd/9g/ggen.c:768: regalloc(&n2, t, N);
use cgenr.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode917
src/cmd/9g/ggen.c:917: r7.op = OREGISTER;
I'm a little confused about what registers are available at this point in the
code.
The 6g code is assuming it might have to save DI elsewhere in order to use DI,
but the 5g code assumes it can just overwrite R0 and R1 without saving them.
It's fine to follow 5g and assume that R7 does not need to be saved, but let's
check
the assumption too.

if(reg[7] > 0)
    fatal("R7 in use during clearfat");

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode973
src/cmd/9g/ggen.c:973: if(debug_checknil && ctxt->debugvlog)
print("expandchecks: %P\n", p); // debug
2 lines

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gobj.c
File src/cmd/9g/gobj.c (left):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gobj.c#oldcode4
src/cmd/9g/gobj.c:4: //	Copyright © 1994-1999 Lucent Technologies Inc.  All
rights reserved.
Restore copyright notice.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c
File src/cmd/9g/gsubr.c (left):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#oldcode3
src/cmd/9g/gsubr.c:3: //
Restore copyright notice.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c
File src/cmd/9g/gsubr.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:524: //switch(as) {
Delete.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:983: //	if(f != N && f->op == OINDEX) {
Delete these.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1001: //case AMOVB:
Delete these.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1034: /*
This should run. Does it not?

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1227: a->reg = 11;
give this register a name. REGCTXT?

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1308: /*	case CASE(OADDR, TPTR32):
delete dead code

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1558: //case CASE(OLROT, TINT8):
This you might want to keep.
Add a TODO.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1600: //case CASE(ORROTC, TINT8):
Add a TODO.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1652: //case CASE(OMOD, TINT8):
Delete

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1666: //case CASE(OMOD, TUINT8):
Delete.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1673: //case CASE(OEXTEND, TINT16):
Delete.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/opt.h
File src/cmd/9g/opt.h (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/opt.h#newcode2
src/cmd/9g/opt.h:2: //
http://code.google.com/p/inferno-os/source/browse/utils/6c/gc.h
copyright notice

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/peep.c
File src/cmd/9g/peep.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/peep.c#newcode2
src/cmd/9g/peep.c:2: //
http://code.google.com/p/inferno-os/source/browse/utils/6c/peep.c
copyright notice

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/peep.c#newcode10
src/cmd/9g/peep.c:10: peep(Prog *p) {
\n{

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/reg.c
File src/cmd/9g/reg.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/reg.c#newcode2
src/cmd/9g/reg.c:2: //
http://code.google.com/p/inferno-os/source/browse/utils/6c/reg.c
copyright notice

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/reg.c#newcode10
src/cmd/9g/reg.c:10: regopt(Prog *p) {
\n{

[minux, 2014-08-12 03:10:58]

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

Please take another look.

[minux, 2014-08-12 03:11:18]

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c
File src/cmd/9g/cgen.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode204
src/cmd/9g/cgen.c:204: // TODO(minux): we shouldn't re-order FP comparisons, but
then we need to synthesize
On 2014/08/11 19:00:35, rsc wrote:
> I don't know what you mean by re-order. I think the code in 6g already does
the
> right thing. I think this can be deleted unless I'm missing something.
Sorry, I actually mean reverse the conditions (ONE -> not OEQ),
due to the way FCMPU is implemented wrt. NaNs, those operations
are not safe and there is a failing test (test/floatcmp.go or something).

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode539
src/cmd/9g/cgen.c:539: case OMUL:
On 2014/08/11 19:00:35, rsc wrote:
> left shift?
you're right. added OLSH.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode818
src/cmd/9g/cgen.c:818: gins(AMOVD, &n1, &n2);
On 2014/08/11 19:00:35, rsc wrote:
> Pretty sure this is not doing an LEAQ equivalent here.
> n1 is tempname, which is a direct reference (name(SB) not $name(SB)).
> Need to add the $ by editing p->from.type I think.
> Or by using an OADDR node like below.
Used the OADDR technique.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode898
src/cmd/9g/cgen.c:898: //ginscon(optoas(OADD, types[tptr]), n->xoffset, res);
On 2014/08/11 19:00:35, rsc wrote:
> It sounds like you should write ginscon instead of duplicating this code.
> ginscon will be a good place to decide about the use of immediate constants.
Yes. Switched to use ginscon()

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode919
src/cmd/9g/cgen.c:919: //ginscon(optoas(OADD, types[tptr]), n->xoffset, res);
same.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode936
src/cmd/9g/cgen.c:936: //ginscon(optoas(OADD, types[tptr]), n->xoffset, res);
same.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1267: // and cmpli accepts 16-bit unsigned immediate.
On 2014/08/11 19:00:36, rsc wrote:
> maybe the TODO is to use ginscon.
9a doesn't accept CMP $a, Rx
so the we need another form of ginscon that take
a constant as to.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1282: /*if(isfloat[nr->type->etype] && (n->op == OEQ || n->op
== ONE)) {
On 2014/08/11 19:00:36, rsc wrote:
> Delete if not needed.
> Add TODO if needed.
Added TODO.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/cgen.c#newcode...
src/cmd/9g/cgen.c:1503: // move
added TODO.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c
File src/cmd/9g/ggen.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/ggen.c#newcode598
src/cmd/9g/ggen.c:598: // A%B = A-(A/B*B)
On 2014/08/11 19:00:36, rsc wrote:
> The power div doesn't give you mod for free? I'm a little surprised.
It doesn't support the MOD operation actually.
and due to the way its instructions are encoded, it's not possible to
add a 2nd output register. (Similar to ARM/SPARC, and unlike MIPS where
divide always output to dedicated HI and LO registers.)

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c
File src/cmd/9g/gsubr.c (right):

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1034: /*
On 2014/08/11 19:00:37, rsc wrote:
> This should run. Does it not? 
pkg/runtime/error.go:60: internal compiler error: bad width: 00906 (245)       
MOVD    $type."".TypeAssertionError(SB),R7 (1, 8)

I updated the code to be able to run, but still commented out, and added a TODO.

https://codereview.appspot.com/123170043/diff/60001/src/cmd/9g/gsubr.c#newcod...
src/cmd/9g/gsubr.c:1227: a->reg = 11;
On 2014/08/11 19:00:37, rsc wrote:
> give this register a name. REGCTXT?
Done. It's called REGENV.

[rsc, 2014-08-12 03:16:04]

LGTM

[minux, 2014-08-12 03:45:01]

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

[dev.power64] cmd/9g: first check point

LGTM=rsc
R=rsc, iant
CC=golang-codereviews
https://codereview.appspot.com/123170043

[gobot, 2014-08-12 05:29:20]

This CL appears to have broken the netbsd-amd64-bsiegert builder.
See http://build.golang.org/log/9bdac8dc1119e72f6e62e5c080ed1b89630adf28

[minux, 2014-08-12 06:42:33]

On Tue, Aug 12, 2014 at 1:29 AM, <gobot@golang.org> wrote:

> This CL appears to have broken the netbsd-amd64-bsiegert builder.
> See http://build.golang.org/log/9bdac8dc1119e72f6e62e5c080ed1b89630adf28

Nope.

# GOMAXPROCS=2 runtime -cpu=1,2,4
--- FAIL: TestFinalizerType-4 (4.62s)
mfinal_test.go:61: finalizer for type func(*int) didn't run
FAIL
FAIL runtime 221.289s