7 ms·

GCC 4.8 Breaks Broken SPEC 2006 Benchmarks (fixed)

48 points by neeee 14y ago

mrich 14y ago
As pointed out in the comments the code is quite contrived to begin with, no surprise such mistakes go unnoticed. As long as a warning is emitted when the compiler relies only such undefined behavior it is a good thing to optimize this heavily, there are not many other ways to get more performance from C/C++.
ginko 14y ago
I'm not really a crack at micro-optimizing C code, so can anyone here explain to me why the given function was written in the form it was? Why not something like this: int d[16]; int SATD (void) { int satd = 0, dd, k; for (k=0; k<16; ++k) { dd = d[k]; satd += (dd < 0 ? -dd : dd); } return satd; } Does this generate slower code?
- cygx 14y ago
  Does this generate slower code? No, or at least not with an optimizing compiler. It was probably more about writing compact code. Personally, I'd have used abs() and gotten rid of the temporary variable dd altogether: int SATD (void) { int satd = 0, k; for (k = 0; k < sizeof d / sizeof *d; ++k) satd += abs(d[k]); return satd; }
  - mansr 14y ago
    Good thinking. Most modern compilers will recognise abs() and generate efficient code.
- Someone 14y ago
  My guess would be: yes, that generated slower code, at the time the code was written. A C compiler from the 1970's was almost little more than a glorified pattern matcher. It easily could emit different code for "i += 1" vs "++i" vs "i++" (and that would not be a problem, as the C programmers used C as a replacement for assembly, and there, "INC R" is different from "ADD #1,R") To check whether that theory holds water, one would have to check older SPEC benchmarks, to see where this fragments came from.
  - mansr 14y ago
    The code in question comes from the reference implementation of the H.264 video codec. What scraps of code this was cobbled together from is anyone's guess.
    - cygx 14y ago
      I tracked it down to http://iphome.hhi.de/suehring/tml/download/old_jm/ http://iphome.hhi.de/suehring/tml/download/old_jm/ The bug is in version 9.3 used by SPEC CPU2006, but no longer in version 9.5. The current version is 18.4...
kzrdude 14y ago
As pointed out by the LWN crowd, the article author is mistaken, because the 4.8 release doesn't do this. http://lwn.net/Articles/544123/ http://lwn.net/Articles/544123/
jedbrown 14y ago
It's disappointing that SPEC refuses to fix the code and is spreading disinformation like "it is at minimum arguable that the code fragment is compliant under the C89 standard". If the global 'int d[16]' is placed at the very end of a page (which the linker is allowed to do), that code compiled in the most naive possible way will SEGV.
- cygx 14y ago
  I wonder where their delusion that C89 allows reading past the end of an array comes from...
  - mikeash 14y ago
    I can only guess that it comes from the fact that it's legal to have a pointer that points on element past the end of the array. That is, this is legal: int array[16]; int *ptr = array + 16; But it's not legal to actually dereference this pointer. The distinction is confusing to a lot of programmers, used to modern architectures where pointers and integers share registers and you can do crazy things with your pointer variables with no consequences. However, some architectures will crash just for having a bad pointer value. That is, this is not legal: int array[16]; int *ptr = array + 17; Even if you never dereference ptr, this code is still illegal, and the compiler is within its rights to generate code that crashes or makes demons fly out of your nose when it encounters this source. The first case is an exception made to make it easier to iterate over an array. So there's a spot in the C standard that says, essentially, "It is OK to have a pointer that points one element past the end of an array." This is probably not too hard to misread as "It is OK to read one element past the end of an array." Although that is definitely wrong.
- mnarayan01 14y ago
  Unless the C89 standard says something about VM interaction (I'm too lazy to check), it seems like your complaint is misguided.
  - cygx 14y ago
    How so? The pointer pointing past the last member of an array may not be dereferenced. Doing so is UB - end of story.
miga 14y ago
Should SPEC pay for static analysis services like Coverity on the code that enters the benchmark? Or does it take a law to make them fix bugs in the code they ship?

48 points by neeee 14y ago

mrich 14y ago

As pointed out in the comments the code is quite contrived to begin with, no surprise such mistakes go unnoticed. As long as a warning is emitted when the compiler relies only such undefined behavior it is a good thing to optimize this heavily, there are not many other ways to get more performance from C/C++.

ginko 14y ago

I'm not really a crack at micro-optimizing C code, so can anyone here explain to me why the given function was written in the form it was? Why not something like this: int d[16]; int SATD (void) { int satd = 0, dd, k; for (k=0; k<16; ++k) { dd = d[k]; satd += (dd < 0 ? -dd : dd); } return satd; } Does this generate slower code?

cygx 14y ago
Does this generate slower code? No, or at least not with an optimizing compiler. It was probably more about writing compact code. Personally, I'd have used abs() and gotten rid of the temporary variable dd altogether: int SATD (void) { int satd = 0, k; for (k = 0; k < sizeof d / sizeof *d; ++k) satd += abs(d[k]); return satd; }
- mansr 14y ago
  Good thinking. Most modern compilers will recognise abs() and generate efficient code.
Someone 14y ago
My guess would be: yes, that generated slower code, at the time the code was written. A C compiler from the 1970's was almost little more than a glorified pattern matcher. It easily could emit different code for "i += 1" vs "++i" vs "i++" (and that would not be a problem, as the C programmers used C as a replacement for assembly, and there, "INC R" is different from "ADD #1,R") To check whether that theory holds water, one would have to check older SPEC benchmarks, to see where this fragments came from.
- mansr 14y ago
  The code in question comes from the reference implementation of the H.264 video codec. What scraps of code this was cobbled together from is anyone's guess.
  - cygx 14y ago
    I tracked it down to http://iphome.hhi.de/suehring/tml/download/old_jm/ http://iphome.hhi.de/suehring/tml/download/old_jm/ The bug is in version 9.3 used by SPEC CPU2006, but no longer in version 9.5. The current version is 18.4...

kzrdude 14y ago

As pointed out by the LWN crowd, the article author is mistaken, because the 4.8 release doesn't do this. http://lwn.net/Articles/544123/ http://lwn.net/Articles/544123/

jedbrown 14y ago

It's disappointing that SPEC refuses to fix the code and is spreading disinformation like "it is at minimum arguable that the code fragment is compliant under the C89 standard". If the global 'int d[16]' is placed at the very end of a page (which the linker is allowed to do), that code compiled in the most naive possible way will SEGV.

cygx 14y ago
I wonder where their delusion that C89 allows reading past the end of an array comes from...
- mikeash 14y ago
  I can only guess that it comes from the fact that it's legal to have a pointer that points on element past the end of the array. That is, this is legal: int array[16]; int *ptr = array + 16; But it's not legal to actually dereference this pointer. The distinction is confusing to a lot of programmers, used to modern architectures where pointers and integers share registers and you can do crazy things with your pointer variables with no consequences. However, some architectures will crash just for having a bad pointer value. That is, this is not legal: int array[16]; int *ptr = array + 17; Even if you never dereference ptr, this code is still illegal, and the compiler is within its rights to generate code that crashes or makes demons fly out of your nose when it encounters this source. The first case is an exception made to make it easier to iterate over an array. So there's a spot in the C standard that says, essentially, "It is OK to have a pointer that points one element past the end of an array." This is probably not too hard to misread as "It is OK to read one element past the end of an array." Although that is definitely wrong.
mnarayan01 14y ago
Unless the C89 standard says something about VM interaction (I'm too lazy to check), it seems like your complaint is misguided.
- cygx 14y ago
  How so? The pointer pointing past the last member of an array may not be dereferenced. Doing so is UB - end of story.

miga 14y ago

Should SPEC pay for static analysis services like Coverity on the code that enters the benchmark? Or does it take a law to make them fix bugs in the code they ship?