Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Sven Wischnowsky]

mason@primenet.com.au wrote:

> Sven Wischnowsky <wischnow@informatik.hu-berlin.de> typed:
> :Geoff Wing wrote:
> :> What about just expanding it slightly (with appropriate comments of course)
> :> instead of duplicating it.
> :> e.g.
> :>     if (*args && **args == '?')
> :>         args++;
> :>     /* default result parameter */
> :>     if (*args)
> :>         reply = *args++;
> :>     else
> :>         reply = ops['A'] ? "reply" : "REPLY";
> :Note this isn't intended as an optimisation (of course), but to work
> :around a bug in gcc-2.8.1 under DU.
> 
> I thought it may be a gcc optimisation bug.  i.e. Does it appear at -O0 or
> -01 or -O2?  All of them?

Yes, it is. The problem is in the first two lines of the code
above. If there is only the `args++' in the `then'-branch, `args'
contains a random value after the `if'. This happens with all
optimisation levels, so I guess it's a register allocation problem in
the backend.

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Sven Wischnowsky]

Peter Stephenson wrote:

> Sven Wischnowsky wrote:
> > Playing some more: both
> > 
> >     firstarg = (*args && **args == '?' ? *args++ : *args);
> > 
> > and
> > 
> >     if (*args && **args == '?')
> >     	firstarg = *args++;
> >     else
> >         firstarg = *args;
> > 
> > work around the bug, too. Should I send a patch for one of these? They 
> > probably keep the code better readable (although they don't look much
> > less silly).
> 
> That might look a bit neater.  You'd better send it relative to the altered
> code, so I don't have to spend all of 30 seconds backing that off.

Yup.

Bye
 Sven

--- os/builtin.c	Fri Jul 16 08:35:14 1999
+++ Src/builtin.c	Fri Jul 16 11:19:35 1999
@@ -3261,16 +3261,10 @@
 	    nchars = 1;
 	args++;
     }
-
-    firstarg = *args;
-    if (*args && **args == '?') {
-    	args++;
-	/* default result parameter */
-	reply = *args ? *args++ : ops['A'] ? "reply" : "REPLY";
-	/* (If we put this reply=... after the `if' gcc-2.8.1 under
-	   Digital Unix 4.0 generates incorrect code.) */
-    } else
-	reply = *args ? *args++ : ops['A'] ? "reply" : "REPLY";
+    /* This `*args++ : *args' looks a bit weird, but it works around a bug
+     * in gcc-2.8.1 under DU 4.0. */
+    firstarg = (*args && **args == '?' ? *args++ : *args);
+    reply = *args ? *args++ : ops['A'] ? "reply" : "REPLY";
 
     if (ops['A'] && *args) {
 	zwarnnam(name, "only one array argument allowed", NULL, 0);

--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Sven Wischnowsky]

I wrote:

> mason@primenet.com.au wrote:
> 
> > Sven Wischnowsky <wischnow@informatik.hu-berlin.de> typed:
> > :Geoff Wing wrote:
> > :> What about just expanding it slightly (with appropriate comments of course)
> > :> instead of duplicating it.
> > :> e.g.
> > :>     if (*args && **args == '?')
> > :>         args++;
> > :>     /* default result parameter */
> > :>     if (*args)
> > :>         reply = *args++;
> > :>     else
> > :>         reply = ops['A'] ? "reply" : "REPLY";
> > :Note this isn't intended as an optimisation (of course), but to work
> > :around a bug in gcc-2.8.1 under DU.
> > 
> > I thought it may be a gcc optimisation bug.  i.e. Does it appear at -O0 or
> > -01 or -O2?  All of them?
> 
> Yes, it is. The problem is in the first two lines of the code
> above. If there is only the `args++' in the `then'-branch, `args'
> contains a random value after the `if'. This happens with all
> optimisation levels, so I guess it's a register allocation problem in
> the backend.

Playing some more: both

    firstarg = (*args && **args == '?' ? *args++ : *args);

and

    if (*args && **args == '?')
    	firstarg = *args++;
    else
        firstarg = *args;

work around the bug, too. Should I send a patch for one of these? They 
probably keep the code better readable (although they don't look much
less silly).

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Peter Stephenson]

Sven Wischnowsky wrote:
> Playing some more: both
> 
>     firstarg = (*args && **args == '?' ? *args++ : *args);
> 
> and
> 
>     if (*args && **args == '?')
>     	firstarg = *args++;
>     else
>         firstarg = *args;
> 
> work around the bug, too. Should I send a patch for one of these? They 
> probably keep the code better readable (although they don't look much
> less silly).

That might look a bit neater.  You'd better send it relative to the altered
code, so I don't have to spend all of 30 seconds backing that off.

-- 
Peter Stephenson <pws@ibmth.df.unipi.it>       Tel: +39 050 844536
WWW:  http://www.ifh.de/~pws/
Dipartimento di Fisica, Via Buonarroti 2, 56127 Pisa, Italy

Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Geoff Wing]

Sven Wischnowsky <wischnow@informatik.hu-berlin.de> typed:
:Geoff Wing wrote:
:> What about just expanding it slightly (with appropriate comments of course)
:> instead of duplicating it.
:> e.g.
:>     if (*args && **args == '?')
:>         args++;
:>     /* default result parameter */
:>     if (*args)
:>         reply = *args++;
:>     else
:>         reply = ops['A'] ? "reply" : "REPLY";
:Note this isn't intended as an optimisation (of course), but to work
:around a bug in gcc-2.8.1 under DU.

I thought it may be a gcc optimisation bug.  i.e. Does it appear at -O0 or
-01 or -O2?  All of them?

Regards,
-- 
Geoff Wing : <gcw@pobox.com>     Work URL: http://www.primenet.com.au/
Rxvt Stuff : <gcw@rxvt.org>      Ego URL : http://pobox.com/~gcw/
Zsh Stuff  : <gcw@zsh.org>       Phone   : (Australia) 0413 431 874

Re: Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Sven Wischnowsky]

Geoff Wing wrote:

> What about just expanding it slightly (with appropriate comments of course)
> instead of duplicating it.
> e.g.
> 
>     if (*args && **args == '?')
>         args++;
>     /* default result parameter */
>     if (*args)
>         reply = *args++;
>     else
>         reply = ops['A'] ? "reply" : "REPLY";

Note this isn't intended as an optimisation (of course), but to work
around a bug in gcc-2.8.1 under DU. And your version doesn't help
here, with that I still get the SEGV. The problem really is in the
first two lines above. It needs more than just the `args++' in after
the `if', otherwise it has a random value for `args' after the it.

Bye
 Sven


--
Sven Wischnowsky                         wischnow@informatik.hu-berlin.de

Bad optimisations: (Was: Test version zsh-3.1.6-test-1)

[Geoff Wing]

Sven Wischnowsky <wischnow@informatik.hu-berlin.de> wrote in zsh-users:
:Under DU 4.0 with gcc-2.8.1 I get a SEGV in a piece of completely
:correct C-code in bin_read(). I can work around this by using the
:patch below -- which, of course, is completely silly.
:Dunno if this should be included, but the SEGV is deadly: it fails on
:e.g. the read in compinit.

-    if (*args && **args == '?')
-	args++;
-    /* default result parameter */
-    reply = *args ? *args++ : ops['A'] ? "reply" : "REPLY";
+    if (*args && **args == '?') {
+    	args++;
+	/* default result parameter */
+	reply = *args ? *args++ : ops['A'] ? "reply" : "REPLY";
+	/* (If we put this reply=... after the `if' gcc-2.8.1 under
+	   Digital Unix 4.0 generates incorrect code.) */
+    } else
+	reply = *args ? *args++ : ops['A'] ? "reply" : "REPLY";
+

What about just expanding it slightly (with appropriate comments of course)
instead of duplicating it.
e.g.

    if (*args && **args == '?')
        args++;
    /* default result parameter */
    if (*args)
        reply = *args++;
    else
        reply = ops['A'] ? "reply" : "REPLY";

Regards,
-- 
Geoff Wing : <gcw@pobox.com>     Work URL: http://www.primenet.com.au/
Rxvt Stuff : <gcw@rxvt.org>      Ego URL : http://pobox.com/~gcw/
Zsh Stuff  : <gcw@zsh.org>       Phone   : (Australia) 0413 431 874