[Bug c++/30245] New: -O2 generates bad code

[Bug c++/30245] New: -O2 generates bad code

[dcb314 at hotmail dot com]

I compiled the following C++ code on a x86_64 machine
without optimisation.

#include <iostream.h>

int
main()
{
        long long n = 1;

        cout << sizeof( n) << endl;
        for (int i = 0; i < 100; ++i)
        {
                cout << n << ' ' << (float) n << '\n';
                n *= 2;
                if (n == 0)
                {
                        break;
                }
        }
        return 0;
}

When the compiled code runs, it is fine.
There are 64 executions of the loop.

However, I added flag -O2, and the code runs differently.
There are 100 executions of the loop.

Suspect optimiser bug.


-- 
           Summary: -O2 generates bad code
           Product: gcc
           Version: 4.3.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
        AssignedTo: unassigned at gcc dot gnu dot org
        ReportedBy: dcb314 at hotmail dot com
  GCC host triplet: x86_64-pc-linux


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[pinskia at gcc dot gnu dot org]

------- Comment #1 from pinskia at gcc dot gnu dot org  2006-12-18 09:39 -------
No, this is undefined code.
since n is a signed type, and the starting n is 1 and it is only multiplied by
2, then it can only be postive and never equal to 0 because overflow for signed
types is undefined.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[pinskia at gcc dot gnu dot org]

------- Comment #2 from pinskia at gcc dot gnu dot org  2006-12-18 09:44 -------
Oh you can use -fwrapv if you want signed type overflow to be defined as
wrapping.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[dcb314 at hotmail dot com]

------- Comment #3 from dcb314 at hotmail dot com  2006-12-18 09:53 -------
(In reply to comment #1)
> then it can only be postive 

Plausible, but I don't think so.

The executed code displays a negative number after about
64 iterations, then displays about thirty zeros.

There is one set bit in local variable n, and it gets moved up
once per iteration.

At the last but one iteration, n goes negative, because
the sign bit is set. 

Then n goes to zero.

Then the test for n == 0 mysteriously fails in the optimiser.

I'd be interested to see what execution you get on your machine
for this code.


-- 

dcb314 at hotmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[pinskia at gcc dot gnu dot org]

------- Comment #4 from pinskia at gcc dot gnu dot org  2006-12-18 09:59 -------
(In reply to comment #3)
> (In reply to comment #1)
> > then it can only be postive 
> 
> Plausible, but I don't think so.

Again signed type overflow is undefined by the C standard so it can do
anything.
So in this case, we optimize out the n == 0 compare.

> 
> The executed code displays a negative number after about
> 64 iterations, then displays about thirty zeros.
> 
> There is one set bit in local variable n, and it gets moved up
> once per iteration.
> 
> At the last but one iteration, n goes negative, because
> the sign bit is set. 

Again once you have a signed overflow, then the behavior is undefined which is
why the n == 0 compare is optimized to true.

> 
> Then n goes to zero.
> 
> Then the test for n == 0 mysteriously fails in the optimiser.

Yes by the VRP because n can never be negative because again signed type
overflow is undefined.  If we do INT_MAX*2, we could still get INT_MAX as
allowed by the C standard.


> I'd be interested to see what execution you get on your machine
> for this code.

I already know what it does.  Use -fwrapv or unsigned types if you want
something which is defined for overflow.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

Re: [Bug c++/30245] New: -O2 generates bad code

[Gabriel Dos Reis]

"dcb314 at hotmail dot com" <gcc-bugzilla@gcc.gnu.org> writes:

| I compiled the following C++ code on a x86_64 machine
| without optimisation.
| 
| #include <iostream.h>
| 
| int
| main()
| {
|         long long n = 1;
| 
|         cout << sizeof( n) << endl;
|         for (int i = 0; i < 100; ++i)
|         {
|                 cout << n << ' ' << (float) n << '\n';
|                 n *= 2;
|                 if (n == 0)
|                 {
|                         break;
|                 }
|         }
|         return 0;
| }
| 
| When the compiled code runs, it is fine.
| There are 64 executions of the loop.
| 
| However, I added flag -O2, and the code runs differently.
| There are 100 executions of the loop.
| 
| Suspect optimiser bug.

Rather, user bug.

Your program contains an undefined behaviour which is provoked by the
overflow of  n *= 2;  the compiler is therefore technically allowed to
do whatever it likes.  In this case, the optimizer assumes that you
can never get from 1 to 0 by successive exponentiation.  Therefore the
test n == 0 is never executed.

-- Gaby


    

[Bug c++/30245] -O2 generates bad code

[gdr at cs dot tamu dot edu]

------- Comment #5 from gdr at cs dot tamu dot edu  2006-12-18 10:16 -------
Subject: Re:   New: -O2 generates bad code

"dcb314 at hotmail dot com" <gcc-bugzilla@gcc.gnu.org> writes:

| I compiled the following C++ code on a x86_64 machine
| without optimisation.
| 
| #include <iostream.h>
| 
| int
| main()
| {
|         long long n = 1;
| 
|         cout << sizeof( n) << endl;
|         for (int i = 0; i < 100; ++i)
|         {
|                 cout << n << ' ' << (float) n << '\n';
|                 n *= 2;
|                 if (n == 0)
|                 {
|                         break;
|                 }
|         }
|         return 0;
| }
| 
| When the compiled code runs, it is fine.
| There are 64 executions of the loop.
| 
| However, I added flag -O2, and the code runs differently.
| There are 100 executions of the loop.
| 
| Suspect optimiser bug.

Rather, user bug.

Your program contains an undefined behaviour which is provoked by the
overflow of  n *= 2;  the compiler is therefore technically allowed to
do whatever it likes.  In this case, the optimizer assumes that you
can never get from 1 to 0 by successive exponentiation.  Therefore the
test n == 0 is never executed.

-- Gaby





-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[gdr at cs dot tamu dot edu]

------- Comment #6 from gdr at cs dot tamu dot edu  2006-12-18 10:58 -------
Subject: Re:  -O2 generates bad code

"dcb314 at hotmail dot com" <gcc-bugzilla@gcc.gnu.org> writes:

| > then it can only be postive 
| 
| Plausible, but I don't think so.

In that case, you might want to read the C++ standard text to
appreciate the facts.

[...]

| I'd be interested to see what execution you get on your machine
| for this code.

what my machine does is irrelevant because your code is invalid.

-- Gaby


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[dcb314 at hotmail dot com]

------- Comment #7 from dcb314 at hotmail dot com  2006-12-19 12:34 -------
(In reply to comment #6)
> In that case, you might want to read the C++ standard text to
> appreciate the facts.

There is a strange sort of echo in this bug report ;->

I accept the point that the code is undefined, according to
the standard and so the compiler is allowed to do anything.

However, the point remains that some perfectly reasonable,
but non standard, code has had its executable action
silently changed in the last few weeks at the -O2 optimisation
level.

This then becomes a quality of implementation issue:

Is it reasonable and prudent to have tests like "if (n == 0)"
optimised away at the -O2 optimisation level ?

We should remember that -O2 is heavily used by lots of
customers.

Some customers might argue that the old behaviour was fine at 
-O2 and such a tense optimisation should only be available at 
higher optimisation levels or moved into a special flag that the 
user can set if they want to from the command line.

There is a clear tradeoff here. If the new behaviour remains,
I suspect the flag -fwrapv will be a popular flag ;->


-- 

dcb314 at hotmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[dberlin at gcc dot gnu dot org]

------- Comment #8 from dberlin at gcc dot gnu dot org  2006-12-19 14:30 -------

> 
> Is it reasonable and prudent to have tests like "if (n == 0)"
> optimised away at the -O2 optimisation level ?
> 
Yes

> We should remember that -O2 is heavily used by lots of
> customers.

And they expect a lot from our optimizers.

> 
> Some customers might argue that the old behaviour was fine at
> -O2 and such a tense optimisation should only be available at
> higher optimisation levels or moved into a special flag that the
> user can set if they want to from the command line.

The problem is that this "old behavior" is simply "luck".  You are simply
*lucky* the optimizer wasn't smart enough to remove the check in all cases.

Now it is.

> 
> There is a clear tradeoff here. If the new behaviour remains,
> I suspect the flag -fwrapv will be a popular flag ;->

We have and will continue to optimize based on what the standard says we can
do, and provide users flags to do otherwise.

You want to do otherwise, so please use the flags.


-- 

dberlin at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[dcb314 at hotmail dot com]

------- Comment #9 from dcb314 at hotmail dot com  2006-12-19 16:37 -------
(In reply to comment #8)
> And they expect a lot from our optimizers.

Surely not at the -O2 level ?

I thought folks serious about optimisation used -O3
and / or a bunch of other flags like -fsomething ?

> The problem is that this "old behavior" is simply "luck".  You are simply
> *lucky* the optimizer wasn't smart enough to remove the check in all cases.
> 
> Now it is.

Continued pleadings that the code is broken as far as the standard
is concerned and should be fixed aren't really helping, I'm afraid.

I've got the message.

The only way I've got at the moment of finding my broken code is 
by observing its run time behaviour, which is expensive.

How about making this a cheaper search by either

a) generating a compile time warning whenever this optimisation
is applied ? That way I can find & fix the problem in my code
at compile time, not run time.

b) Please identify to me which piece of source code of the compiler is making
this optimisation and I'll either remove the optimisation or put in a warning
msg in my copy of the compiler.

> We have and will continue to optimize based on what the standard says we can
> do, and provide users flags to do otherwise.

Adhering to the standard is all well and good, but the compiler also
has to compile real world code in a reasonable fashion too.

> You want to do otherwise, so please use the flags.

It is very helpful that the flags are provided for those 
who want the old behaviour.


-- 

dcb314 at hotmail dot com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |UNCONFIRMED
         Resolution|INVALID                     |


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[pinskia at gcc dot gnu dot org]

------- Comment #10 from pinskia at gcc dot gnu dot org  2006-12-19 17:40 -------
Again use -fwrapv and forget about it.


-- 

pinskia at gcc dot gnu dot org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|                            |INVALID


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[dcb314 at hotmail dot com]

------- Comment #11 from dcb314 at hotmail dot com  2006-12-19 22:03 -------
(In reply to comment #10)
> Again use -fwrapv and forget about it.

Sigh. This implies using -fwrapv everywhere.

Still relatively expensive - I still don't know where the compiler
is doing the new behaviour.

For an additional data point, I tried out the Intel C compiler
at optimisation level -O2 *and* -O3 on the demonstration
source code.

Clearly, different compiler writers have different tradeoffs.

However, Intel produced the old GNU behaviour at both -O2 *and* -O3.

Of course, one compiler doing it one way does not imply all compilers
have to do it that way.


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245

[Bug c++/30245] -O2 generates bad code

[pinskia at gcc dot gnu dot org]

------- Comment #12 from pinskia at gcc dot gnu dot org  2006-12-19 22:05 -------
See also:
http://gcc.gnu.org/ml/gcc/2006-12/msg00459.html


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=30245