code review 85200043: cmd/5g, cmd/6g, cmd/8g: preserved wide values in large ...

cmd/5g, cmd/6g, cmd/8g: preserve wide values in large functions

In large functions with many variables, the register optimizer
may give up and choose not to track certain variables at all.
In this case, the "nextinnode" information linking together
all the words from a given variable will be incomplete, and
the result may be that only some of a multiword value is
preserved across a call. That confuses the garbage collector,
so don't do that. Instead, mark those variables as having
their address taken, so that they will be preserved at all
calls. It's overkill, but correct.

Tested by hand using the 6g -S output to see that it does fix
the buggy generated code leading to the issue 7726 failure.

There is no automated test because I managed to break the
compiler while writing a test (see issue 7727). I will check
in a test along with the fix to issue 7727.

Fixes issue 7726.

[rsc, 2014-04-08 03:00:44]

Hello khr (cc: golang-codereviews@googlegroups.com),

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

[bradfitz, 2014-04-08 03:17:17]

s/preserved/preserve/ in subject?



On Mon, Apr 7, 2014 at 8:00 PM, <rsc@golang.org> wrote:

> Reviewers: khr,
>
> Message:
> Hello khr (cc: golang-codereviews@googlegroups.com),
>
> I'd like you to review this change to
> https://code.google.com/p/go/
>
>
> Description:
> cmd/5g, cmd/6g, cmd/8g: preserved wide values in large functions
>
> In large functions with many variables, the register optimizer
> may give up and choose not to track certain variables at all.
> In this case, the "nextinnode" information linking together
> all the words from a given variable will be incomplete, and
> the result may be that only some of a multiword value is
> preserved across a call. That confuses the garbage collector,
> so don't do that. Instead, mark those variables as having
> their address taken, so that they will be preserved at all
> calls. It's overkill, but correct.
>
> Tested by hand using the 6g -S output to see that it does fix
> the buggy generated code leading to the issue 7726 failure.
>
> There is no automated test because I managed to break the
> compiler while writing a test (see issue 7727). I will check
> in a test along with the fix to issue 7727.
>
> Fixes issue 7726.
>
> Please review this at https://codereview.appspot.com/85200043/
>
> Affected files (+31, -0 lines):
>   M src/cmd/5g/reg.c
>   M src/cmd/6g/reg.c
>   M src/cmd/8g/reg.c
>
>
> Index: src/cmd/5g/reg.c
> ===================================================================
> --- a/src/cmd/5g/reg.c
> +++ b/src/cmd/5g/reg.c
> @@ -801,6 +801,16 @@
>         if(nvar >= NVAR) {
>                 if(debug['w'] > 1 && node)
>                         fatal("variable not optimized: %D", a);
> +
> +               // If we're not tracking a word in a variable, mark the
> rest as
> +               // having its address taken, so that we keep the whole
> thing
> +               // live at all calls. otherwise we might optimize away
> part of
> +               // a variable but not all of it.
> +               for(i=0; i<nvar; i++) {
> +                       v = var+i;
> +                       if(v->node == node)
> +                               v->addr = 1;
> +               }
>                 goto none;
>         }
>
> Index: src/cmd/6g/reg.c
> ===================================================================
> --- a/src/cmd/6g/reg.c
> +++ b/src/cmd/6g/reg.c
> @@ -659,6 +659,17 @@
>         if(nvar >= NVAR) {
>                 if(debug['w'] > 1 && node != N)
>                         fatal("variable not optimized: %#N", node);
> +
> +               // If we're not tracking a word in a variable, mark the
> rest as
> +               // having its address taken, so that we keep the whole
> thing
> +               // live at all calls. otherwise we might optimize away
> part of
> +               // a variable but not all of it.
> +               for(i=0; i<nvar; i++) {
> +               continue;
> +                       v = var+i;
> +                       if(v->node == node)
> +                               v->addr = 1;
> +               }
>                 goto none;
>         }
>
> Index: src/cmd/8g/reg.c
> ===================================================================
> --- a/src/cmd/8g/reg.c
> +++ b/src/cmd/8g/reg.c
> @@ -625,6 +625,16 @@
>         if(nvar >= NVAR) {
>                 if(debug['w'] > 1 && node != N)
>                         fatal("variable not optimized: %D", a);
> +
> +               // If we're not tracking a word in a variable, mark the
> rest as
> +               // having its address taken, so that we keep the whole
> thing
> +               // live at all calls. otherwise we might optimize away
> part of
> +               // a variable but not all of it.
> +               for(i=0; i<nvar; i++) {
> +                       v = var+i;
> +                       if(v->node == node)
> +                               v->addr = 1;
> +               }
>                 goto none;
>         }
>
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.
>

[rsc, 2014-04-08 03:38:08]

already done. you won't see any more rietveld-generated mails on this thread

[khr, 2014-04-09 23:59:00]

LGTM.

[dave_cheney.net, 2014-04-16 00:49:45]

ping

[rsc, 2014-04-16 00:53:30]

I am not 100% sure this is correct. I want to play with it a little more,
but I'm planning to do so real soon now.

[rsc, 2014-04-16 17:59:25]

My hesitation about this was due to 6g crashing on an input file I wrote (issue
7727), but I cannot reproduce that now. I think my client was just in an
inconsistent state. This change still looks correct to me, and it does fix the
assembly generated for the cases I've checked.

[rsc, 2014-04-16 17:59:46]

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

cmd/5g, cmd/6g, cmd/8g: preserve wide values in large functions

In large functions with many variables, the register optimizer
may give up and choose not to track certain variables at all.
In this case, the "nextinnode" information linking together
all the words from a given variable will be incomplete, and
the result may be that only some of a multiword value is
preserved across a call. That confuses the garbage collector,
so don't do that. Instead, mark those variables as having
their address taken, so that they will be preserved at all
calls. It's overkill, but correct.

Tested by hand using the 6g -S output to see that it does fix
the buggy generated code leading to the issue 7726 failure.

There is no automated test because I managed to break the
compiler while writing a test (see issue 7727). I will check
in a test along with the fix to issue 7727.

Fixes issue 7726.

LGTM=khr
R=khr, bradfitz, dave
CC=golang-codereviews
https://codereview.appspot.com/85200043