Compiler bug?

[Geert_Jan_B_]

Dear all,

I stumbled upon something that looks suspiciously like a bug in Intel icc/icpc compiler.  I've tested this with all versions of the compiler I've access to, i.e., 13.1.3 20130607, 15.0.1 20141023, 16.0.2 20160204, 16.0.3 20160415, 17.0.0 20160315, and on a range of hardware, i.e., i5-3210M, E5-2680 v2, E5-2680 v3, E5-2680 v4.

Compilation switches are pretty much irrelevant, typically -O2, no warnings are generated by the compiler.

The complete code is added as an attachment, however, the offending construct is:

...
static double __arg__;
#define SQR(a) ((__arg__ = (a)) == 0.0 ? 0.0 : __arg__*__arg__)
...
    for (i = 0; i < n; i++)
        a = SQR(b) + SQR(c);
...

When the macro definition is replaced by the following, and the global variable __arg__ removed, the results are as expected:

#define SQR(a) ((a) == 0.0 ? 0.0 : (a)*(a))

The following is the output of the program with the original macro:

sum = 166581086880546.25
sum = 224904286352832.38

Both numbers should be equal, so it is clear that the output is complete nonsense.

Compiling this code with both GCC (4.9.2 and 5.4) and clang (3.8) produces the correct result.  However, gcc 5.4 and clang 3.8 generate a warning:

icc_test.c:7:26: warning: operation on ‘__arg__’ may be undefined [-Wsequence-point]
 #define SQR(a) ((__arg__ = (a)) == 0.0 ? 0.0 : __arg__*__arg__)
                          ^
icc_test.c:29:16: note: in expansion of macro ‘SQR’
         a = SQR(b) + SQR(c);
                ^

Also attached is a make file (Makefile.txt, sorry for the extension, otherwise it's not accepted) that will produce two executables, icc_fail.exe, and icc_succeed.exe.

Thanks, best regards, Geert Jan Bex

[Oscar_Creus]

Hi,

It doesn't seems correct to me, maybe I'm wrong, but:

#define SQR(a) ((a) == 0.0 ? 0.0 : (a)*(a))

Operates only with macro parameter (a). The first macro doesn't:

define SQR(a) ((__arg__ = (a)) == 0.0 ? 0.0 : __arg__*__arg__)

Uses the global variable __arg__. You're assuming that first instruction must be __arg__ = a ... which maybe not true, as compiler is warning you: operation on ‘__arg__’ may be undefined [-Wsequence-point]

Avoid those assumptions, or use an inline function for this kind of macros, which C++ recomends to minimize.

inline float 

SQL (float a)

{

return a*a; // I don't see the need for 0 check here, sorry.

}

[Geert_Jan_B_]

Dear,

Thanks for your input.  However, please note that the Intel compiler does not generate a warning, and generates bad code.  This is the point of my post.  GCC (and clang) generate the warning, and the correct code.  Hence this is tagged as a bug report.

The code is quite flaky, I wholeheartedly agree, but it is some legacy code (Numerical Recipes to be precise).  I've already warned the researcher off that code which as written for 386 in the early 90s.

Best regards, Geert Jan Bex

[SergeyKostrov]

By the way, even Turbo C++ compiler v3.0.0 ( 25-year-old software! ) generated correct codes and produced equal results for both test cases! >>...please note that the Intel compiler does not generate a warning, and generates bad code... I support Geert 's statement and there is clearly a problem with the compiler. Here is a little bit modified test case ( n iwas set to 1000 ): #include #include #ifdef FAIL static double __arg__; #define SQR(a) ((__arg__ = (a)) == 0.0 ? 0.0 : __arg__*__arg__) #else #define SQR(a) ((a) == 0.0 ? 0.0 : (a)*(a)) #endif int main(int argc, char *argv[]) { double *a, *b, *c, sum; int n = 1000, i; a = (double *) malloc(n*sizeof(double)); b = (double *) malloc(n*sizeof(double)); c = (double *) malloc(n*sizeof(double)); sum = 0.0; for (i = 0; i < n; i++) { b = (rand() % 2) ? 0.01*i : 0.0; c = (rand() % 2) ? 0.03*i : 0.0; sum += b*b + c*c; } printf("sum = %.2lf\n", sum); for (i = 0; i < n; i++) a = SQR(b) + SQR(c); sum = 0.0; for (i = 0; i < n; i++) sum += a; printf("sum = %.2lf\n", sum); free(a); free(b); free(c); return EXIT_SUCCESS; } Output: C:\WorkTemp\Temp>TESTAPP.EXE sum = 170135.52 sum = 170135.52

[Oscar_Creus]

Hi all!

The answer of a black belt is always the right one :-)

But just trying to help a little bit:

I always use compiler with /W5 level warning, and Intel 15.0 warns about this:

1>testI.cpp(34): message #1572: floating-point equality and inequality comparisons are unreliable
1>        a = SQR(b) + SQR(c);
1>               ^
1>  
1>testI.cpp(34): message #1572: floating-point equality and inequality comparisons are unreliable
1>        a = SQR(b) + SQR(c);
1>                           ^
1>  
1>testI.cpp(34): message #981: operands are evaluated in unspecified order
1>        a = SQR(b) + SQR(c);

Check this: >testI.cpp(34): message #981: operands are evaluated in unspecified order

That's what I'm saying. It may be a compiler problem, but it doens't seems to me, sorry. I think the compiler is a tool, and intelligence must be provided by the programmer (NO offense to anyone, just my thoughts). 

I had to migrate a lot of code, and preciselly for that I always activate the /W5 warning level.

Hope this helps!

[Geert_Jan_B_]

Thanks for the tip, I didn't know about the /W5.

Hm, this is somewhat bizarre.  The Linux version of 15.0.1 20141023 (and 17.0.0 20160315 for that matter) do generate the warning about comparing floating point numbers (#1572), but not the one on the unspecified evaluation order (#981).  The latter is indeed the crucial one, and it is generated by GCC and clang as I mentioned.

I compile with:

icc -Wall -Wremarks -Wchecks -w3 ...

For the Linux version, -w3 is as good as it gets, there is no -w5.  If anyone can give me a hint on how to produce the #981 on Linux, I'd be grateful.  It was indeed thanks to that warning generated by GCC that I was able to solve the problem for the researchers involved in the first place.

No offence taken, the code is 35 years old, written for processors that didn't have the features that would mess up this code.  Most importantly, it was not written by me ;)

However, unless someone can actually produce #981, this thing still wriggles its antennae ;)

Best regards, Geert Jan Bex