code review 5606061: cmd/gc: fix codegen reordering for expressions involvin...

cmd/gc: fix codegen reordering for expressions involving && and ||

Fixes issue 2821.

[lvd, 2012-02-03 10:27:05]

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

I'd like you to review this change to
https://go.googlecode.com/hg/

[rsc, 2012-02-03 14:51:34]

Ouch.  Thanks for finding this.  It's a little odd to
single out OANDAND and OOROR as having possibly
complex children; a simple OADD might have a call
inside it, but we don't penalize all the OADDs for that.
The code in the general case, which considers the two
children, is supposed to handle this case.  It might
be cleaner and easier to understand to add
    if(n->ninit) {
        ul = UINF;
        goto out;
    }
before or after the switch instead.

That said, that was one of the first things I tried when
I saw this report, and it didn't work for me, but maybe
I confused myself about which compiler I was really
running.  If that doesn't work but the edit in this CL does,
we should add a comment explaining those two cases
very carefully.

Thanks.
Russ

[lvd, 2012-02-03 15:03:11]

On Fri, Feb 3, 2012 at 15:51, Russ Cox <rsc@golang.org> wrote:
> Ouch.  Thanks for finding this.  It's a little odd to
> single out OANDAND and OOROR as having possibly

i think it only hurts b/c ANDAND and OROR get handled by bgen.
Diffing the -W output i noticed that reorder1 had swapped the OANDAND
in i = true && expr
in the generated call to convT2E with the 'type.bool' argument.
diffing the -S output i noticed that the assembly code for that that
had the type argument being set up in the new code, then the bgen
generated OAND branching, and then that call, by wich time the
'type.bool' pointer was clobbered.

> complex children; a simple OADD might have a call
> inside it, but we don't penalize all the OADDs for that.
> The code in the general case, which considers the two
> children, is supposed to handle this case.  It might
> be cleaner and easier to understand to add
>    if(n->ninit) {
>        ul = UINF;
>        goto out;
>    }

i'll try.  i'll also try triggering the bug with an OADD

> before or after the switch instead.
>
> That said, that was one of the first things I tried when
> I saw this report, and it didn't work for me, but maybe
> I confused myself about which compiler I was really
> running.

the go tool made everything easier, including confusing oneself what
compiler is running

> If that doesn't work but the edit in this CL does,
> we should add a comment explaining those two cases
> very carefully.
>
> Thanks.
> Russ

[lvd, 2012-02-03 16:18:27]

btw its not that the andand has a potential ninit, its that its ->rigth
gets reordered in place.
On Feb 3, 2012 4:03 PM, "Luuk van Dijk" <lvd@google.com> wrote:

> On Fri, Feb 3, 2012 at 15:51, Russ Cox <rsc@golang.org> wrote:
> > Ouch.  Thanks for finding this.  It's a little odd to
> > single out OANDAND and OOROR as having possibly
>
> i think it only hurts b/c ANDAND and OROR get handled by bgen.
> Diffing the -W output i noticed that reorder1 had swapped the OANDAND
> in i = true && expr
> in the generated call to convT2E with the 'type.bool' argument.
> diffing the -S output i noticed that the assembly code for that that
> had the type argument being set up in the new code, then the bgen
> generated OAND branching, and then that call, by wich time the
> 'type.bool' pointer was clobbered.
>
> > complex children; a simple OADD might have a call
> > inside it, but we don't penalize all the OADDs for that.
> > The code in the general case, which considers the two
> > children, is supposed to handle this case.  It might
> > be cleaner and easier to understand to add
> >    if(n->ninit) {
> >        ul = UINF;
> >        goto out;
> >    }
>
> i'll try.  i'll also try triggering the bug with an OADD
>
> > before or after the switch instead.
> >
> > That said, that was one of the first things I tried when
> > I saw this report, and it didn't work for me, but maybe
> > I confused myself about which compiler I was really
> > running.
>
> the go tool made everything easier, including confusing oneself what
> compiler is running
>
> > If that doesn't work but the edit in this CL does,
> > we should add a comment explaining those two cases
> > very carefully.
> >
> > Thanks.
> > Russ
>

[rsc, 2012-02-03 16:25:16]

Right so its right should have a ninit and be UINF which should percolate
up.

[lvd, 2012-02-06 10:38:46]

On 2012/02/03 16:25:16, rsc wrote:
> Right so its right should have a ninit and be UINF which should percolate
> up.

the conv doesn't get the ninit until after it's been ulmanncalc'ed.  looking for
the best fix...

[lvd, 2012-02-06 10:49:31]

On 2012/02/06 10:38:46, lvd wrote:
> On 2012/02/03 16:25:16, rsc wrote:
> > Right so its right should have a ninit and be UINF which should percolate
> > up.
> 
> the conv doesn't get the ninit until after it's been ulmanncalc'ed.  looking
for
> the best fix...

patch set 4 works, but maybe it would be better to pull the entire ullmancalc
into order.

[rsc, 2012-02-06 14:34:44]

LGTM

[lvd, 2012-02-06 14:41:06]

*** Submitted as http://code.google.com/p/go/source/detail?r=58badc8d57f6 ***

cmd/gc: fix codegen reordering for expressions involving && and ||

Fixes issue 2821.

R=rsc
CC=golang-dev
http://codereview.appspot.com/5606061