Bug 53073 - [4.8 Regression] 464.h264ref in SPEC CPU 2006 miscompiled
Summary: [4.8 Regression] 464.h264ref in SPEC CPU 2006 miscompiled
Status: RESOLVED INVALID
Alias: None
Product: gcc
Classification: Unclassified
Component: middle-end (show other bugs)
Version: 4.8.0
: P3 normal
Target Milestone: 4.8.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
: 55216 (view as bug list)
Depends on:
Blocks: 53086
  Show dependency treegraph
 
Reported: 2012-04-22 17:10 UTC by H.J. Lu
Modified: 2013-01-31 09:28 UTC (History)
6 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed:


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description H.J. Lu 2012-04-22 17:10:37 UTC
On Linux/x86-64, revision 186592:

http://gcc.gnu.org/ml/gcc-cvs/2012-04/msg00543.html

compiles 464.h264ref in SPEC CPU 2006 into infinite loop.
Comment 1 Richard Biener 2012-04-23 09:20:37 UTC
Well, I'm sure it is h264refs fault.
Comment 2 Richard Biener 2012-04-23 11:29:58 UTC
1016    int
1017    SATD (int* diff, int use_hadamard)
1018    {
1019      int k, satd = 0, m[16], dd, d[16];
...
1092        /*===== sum up =====*/
1093        for (dd=d[k=0]; k<16; dd=d[++k])
1094        {
1095          satd += (dd < 0 ? -dd : dd);
1096        }

accesses d[16].
Comment 3 Richard Biener 2012-04-23 11:44:11 UTC
Fixed by doing

    /*===== sum up =====*/
    for (k=0; k < 16; k++)
    {
      dd = d[k];
      satd += (dd < 0 ? -dd : dd);
    }

instead.
Comment 4 Venkataramanan 2012-08-21 06:27:29 UTC
is there are src.alts available for this benchmark ?
Comment 5 Andrew Pinski 2012-11-08 10:42:25 UTC
*** Bug 55216 has been marked as a duplicate of this bug. ***
Comment 6 Michael Paton 2013-01-15 17:02:31 UTC
I would like to question the resolution status of this bug.

Perhaps I will be educated on the process used here, but I fail to see how supplying different source code fixes this bug. Making it go away, IMO is not the same as fixing it.

The original code is likely not what the original author intended, and the alternate code supplied by Richard Biener is by some measures "better".

However the original code is NOT invalid C code. It certainly reads memory past the end of an array, and so assigns an undetermined value to the variable dd. However this action is

1 Still valid C code
2 does not affect the control flow of the for loop in the code as written

Furthermore, the "endless loop" is in fact a jump to the current instruction, with no sign of code for the accesses of arrays d and m.

While it is inconvenient for the optimizer to discover source code like this, the optimizer is free to either not optimize, or complain via a diagnostic, or in the extreme to refuse to compile. I do not believe it is free to silently emit bad code in response to inconvenient, valid C ode.

I'd be happy to be educated on why the emitted code could be valid; currently I do not see how it could be.
Comment 7 Jakub Jelinek 2013-01-15 17:56:05 UTC
You're wrong, the code in #c2 is not valid C.
ISO C99 6.5.2.1 says that d[++k] is equivalent to:
  (*((d)+(++k)))
and ++k in the last iteration is 16, so it is
  (*(d+16))
and then 6.5.6/8 (last sentence) applies:
"If the result points one past the last element of the array object, it
shall not be used as the operand of a unary * operator that is evaluated."
So, if you ever enter this loop, you'll invoke undefined behavior and anything can happen.
Comment 8 Michael Paton 2013-01-15 20:01:04 UTC
Hopefully it's correct to reply to the mailing list rather than to bugzilla.
I've tried to find definitive guidance on that and failed.

Thanks for your informative reply asserting C99 non compliance. My assertion on
validity was C89 based, but let's use yours and move on.

I accept that the source code in #c2 is invalid C99 code, and that undefined
behavior may be the result.

However, I will still argue that undefined behavior should be a diagnostic, or a
lack of an optimizing transformation. If the compiler is to be silent on
discovering this invalid C code, I do not believe it should go on to produce
code that runs until it starts executing an infinite loop.

While that may be allowed by ISO C99, I have a hard time believing that it is
useful to the users of gcc, or indeed intended by its contributors. This invalid
code compiles and executes with -O0, yet generates bad assembly code ending in
an infinite loop with -O2. I think this is indicative of a bug in gcc 4.8.0. As
to whether such a bug might influence valid code, I cannot say.

Michael Paton

On 1/15/2013 11:56 AM, jakub at gcc dot gnu.org wrote:
> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53073
> 
> Jakub Jelinek <jakub at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |jakub at gcc dot gnu.org
> 
> --- Comment #7 from Jakub Jelinek <jakub at gcc dot gnu.org> 2013-01-15 17:56:05 UTC ---
> You're wrong, the code in #c2 is not valid C.
> ISO C99 6.5.2.1 says that d[++k] is equivalent to:
>   (*((d)+(++k)))
> and ++k in the last iteration is 16, so it is
>   (*(d+16))
> and then 6.5.6/8 (last sentence) applies:
> "If the result points one past the last element of the array object, it
> shall not be used as the operand of a unary * operator that is evaluated."
> So, if you ever enter this loop, you'll invoke undefined behavior and anything
> can happen.
>
Comment 9 Steve Ellcey 2013-01-28 18:50:42 UTC
FYI: I reported this issue to SPEC to make sure they were aware of it.
their reply is at:

http://www.spec.org/cpu2006/Docs/faq.html#Run.05

They do not intend to offer alternative sources.
Comment 10 rguenther@suse.de 2013-01-29 09:10:25 UTC
On Mon, 28 Jan 2013, sje at gcc dot gnu.org wrote:

> 
> http://gcc.gnu.org/bugzilla/show_bug.cgi?id=53073
> 
> Steve Ellcey <sje at gcc dot gnu.org> changed:
> 
>            What    |Removed                     |Added
> ----------------------------------------------------------------------------
>                  CC|                            |sje at gcc dot gnu.org
> 
> --- Comment #9 from Steve Ellcey <sje at gcc dot gnu.org> 2013-01-28 18:50:42 UTC ---
> FYI: I reported this issue to SPEC to make sure they were aware of it.
> their reply is at:
> 
> http://www.spec.org/cpu2006/Docs/faq.html#Run.05
> 
> They do not intend to offer alternative sources.

So be it.  At a last resort we could add a switch that disables
just number-of-iteration computations based on undefined behavior.
Note that all previous releases of GCC have the same behavior - the
knowledge is just not used.  Thus such switch would pessimize
code further than reverting to previous behavior - nevertheless
such switch would be consistent with existing switches like
-fno-strict-aliasing or -fno-strict-overflow.

Richard.
Comment 11 Richard Biener 2013-01-31 09:01:02 UTC
Author: rguenth
Date: Thu Jan 31 09:00:54 2013
New Revision: 195610

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=195610
Log:
2013-01-31  Richard Biener  <rguenther@suse.de>

	PR middle-end/53073
	* common.opt (faggressive-loop-optimizations): New flag,
	enabled by default.
	* doc/invoke.texi (faggressive-loop-optimizations): Document.
	* tree-ssa-loop-niter.c (estimate_numbers_of_iterations_loop): Guard
	infer_loop_bounds_from_undefined by it.

Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/common.opt
    trunk/gcc/doc/invoke.texi
    trunk/gcc/tree-ssa-loop-niter.c
Comment 12 Jakub Jelinek 2013-01-31 09:28:28 UTC
FYI, regarding validity in C89, the wording there is similar to C99.
ISO C89, 6.3.1.2 says that d[++k] is equivalent to:
  (*((d)+(++k)))
and ++k in the last iteration is 16, so it is
  (*(d+16))
and then 6.3.6 part applies:
"Unless both the pointer operand and the result point to elements of the
same array object, or the pointer operand points one past the last element  
of an array object and the result points to an element of the same array
object, the behavior is undefined if the result is used as an operand of the 
unary * operator."
Here the pointer operand points to the first element of array (&d[0]) and
the result points one past the last element of the array object, so again, the behavior is undefined.