Bug 30980 - [4.3 Regression] Recent complex miscompilation
Summary: [4.3 Regression] Recent complex miscompilation
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: target (show other bugs)
Version: 4.3.0
: P1 normal
Target Milestone: 4.3.0
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks: 30969
  Show dependency treegraph
 
Reported: 2007-02-27 12:38 UTC by Paolo Carlini
Modified: 2018-05-19 13:46 UTC (History)
6 users (show)

See Also:
Host:
Target: powerpc-apple-darwin8.8.0
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 Paolo Carlini 2007-02-27 12:38:53 UTC
Recently (~10 days ago), 26_numerics/complex/13450.cc started failing for *many* targets, among which powerpc-darwin. On the latter I'm seeing a miscompilation at -O1 and above of this reduced C++ snippet:

#include <math.h>
#include <stdio.h>

int main()
{
  __complex__ double x;

  __real__ x = -1.0;
  __imag__ x = 0.0;

  __complex__ double t = __builtin_clog(x);

  double tmp = 0.5 * (__imag__ t);

  printf("%g %g\n", cos(tmp), sin(tmp));
}

/////////////////

Note how the output changes at -O1 and above. Also note that the very same snippet, as C is fine, thus I'm categorizing as C++, not as middle-end or something else.
Comment 1 Paolo Carlini 2007-02-27 13:20:56 UTC
Looking at testresults, it seems 26_numerics/complex/13450.cc started failing on powerpc-apple-darwin8.5.0 between 122031 and 122067.
Comment 2 Richard Biener 2007-02-27 13:27:09 UTC
As I cannot reproduce this on any linux target I guess the following change did it:

+2007-02-16  Geoffrey Keating  <geoffk@apple.com>
+
+       * config/darwin.h (LINK_SPEC): Always pass -macosx_version_min
+       to linker.
+       (DARWIN_EXTRA_SPECS): Add %(darwin_minversion).
+       * config/rs6000/darwin.h (SUBTARGET_OVERRIDE_OPTIONS): Just call
+       darwin_rs6000_override_options.
+       (C_COMMON_OVERRIDE_OPTIONS): Expect
+       darwin_macosx_version_min to be non-NULL always.
+       (TARGET_C99_FUNCTIONS): Likewise.
+       (CC1_SPEC): Always pass -mmacosx-version-min to cc1*.
+       (DARWIN_MINVERSION_SPEC): New.
+       * config/rs6000/rs6000.c (darwin_rs6000_override_options): New.
+       * config/i386/darwin.h (CC1_SPEC): Always pass -mmacosx-version-min
+       to cc1*.  
+       (DARWIN_MINVERSION_SPEC): New.
+       * config/darwin.opt (mmacosx-version-min): Initialize to non-NULL
+       value.
+       * config/darwin-c.c (darwin_cpp_builtins): Expect
+       darwin_macosx_version_min to be non-NULL always.
+
+       * config/rs6000/rs6000.c: Clean up trailing whitespace.
Comment 3 Paolo Carlini 2007-02-27 13:30:12 UTC
Yes, I can confirm it cannot be reproduced on linux targets. Anyway, we can easily nail down between 122051 and 122067 (from testresults).
Comment 4 Paolo Carlini 2007-02-27 13:39:02 UTC
Note, however, that per Kaveh' reports, around the same dates (between 2007-02-14 and 2007-02-19) it started failing also on sparc-sun-solaris2.10...
Comment 5 Paolo Carlini 2007-02-27 13:41:42 UTC
I'm adding Eric too, maybe he wants to investigate the sparc version of the issue.
Comment 6 Andrew Pinski 2007-02-27 15:42:53 UTC
PR 30969 is related.  The reduced fortran looks like the reduced C++.
Comment 7 Andrew Pinski 2007-02-27 15:47:56 UTC
>  I'm adding Eric too, maybe he wants to investigate the sparc version of the
> issue.

The patch which I was worried about causing a regression with respect of calling complex functions is:
2007-02-18  Sandra Loosemore  <sandra@codesourcery.com>
        
        * calls.c (initialize_argument_information): Pass original EXP
        and STRUCT_VALUE_ADDR_VALUE instead of a list of arguments.  Move
        code to split complex arguments here, as part of initializing the
        ARGS array.
        (expand_call): Remove code that builds a list of arguments and
        inserts implicit arguments into it.  Instead, just count how many
        implicit arguments there will be so we can determine the size of
        the ARGS array, and let initialize_argument_information do the work.
        (split_complex_values): Delete unused function.


Now I don't know if that patch caused the sparc regression or not.

 
Comment 8 Geoff Keating 2007-02-27 17:25:23 UTC
I'm confident that my patch could not possibly have affected targets other than Darwin, and should not have significantly affected code generation even there.  Complex arithmetic testcases will fail for other reasons on Darwin (like PR 24577).
Comment 9 Dominique d'Humieres 2007-03-11 12:19:13 UTC
I confirm that the bug does not show with gcc. Is theis related to the following failures in the regtest?

FAIL: gcc.dg/builtins-59.c scan-tree-dump __builtin_cexpi
FAIL: gcc.dg/builtins-59.c scan-tree-dump-not sincos
FAIL: gcc.dg/builtins-61.c scan-tree-dump cexpi
FAIL: gcc.dg/builtins-61.c scan-tree-dump sin
FAIL: gcc.dg/builtins-61.c scan-tree-dump cos
FAIL: gcc.dg/builtins-61.c scan-tree-dump return 0.0
FAIL: gcc.dg/builtins-62.c scan-tree-dump-times cexpi 3

Also the following fortran code

complex(8) :: cplx, t
real(8)    :: tmp
cplx = cmplx(-1.0,0.0,8)
t = log(cplx)
tmp = 0.5 * imag(t)
print *, cos(tmp), sin(tmp)
end

reproduces the bug.
Comment 10 Dominique d'Humieres 2007-03-13 12:56:47 UTC
The problem seems to come from a broken/unavailable __builtin_cexpi,
see PR31161.

My understanding is that __builtin_cexpi and __builtin_sincos are twin
objects(?). Now I see in gcc/tree-ssa-math-opts.c:

...
static bool       
gate_cse_sincos (void)
{                   
  /* Make sure we have either sincos or cexp.  */
  return (TARGET_HAS_SINCOS
          || TARGET_C99_FUNCTIONS)
         && optimize;
}

struct tree_opt_pass pass_cse_sincos =
{
  "sincos",                             /* name */
  gate_cse_sincos,                      /* gate */
  execute_cse_sincos,                   /* execute */
  NULL,                                 /* sub */
  NULL,                                 /* next */
  0,                                    /* static_pass_number */
  0,                                    /* tv_id */
  PROP_ssa,                             /* properties_required */
  0,                                    /* properties_provided */
  0,                                    /* properties_destroyed */
  0,                                    /* todo_flags_start */
  TODO_dump_func | TODO_update_ssa | TODO_verify_ssa
    | TODO_verify_stmts,                /* todo_flags_finish */
  0                                     /* letter */
};
...

And TARGET_C99_FUNCTIONS is set to 1 on Darwin -> gate_cse_sincos returns
true even if TARGET_HAS_SINCOS is 0.
Comment 11 Richard Biener 2007-03-13 13:22:10 UTC
Because if TARGET_C99_FUNCTIONS is set we can expand __builtin_cexpi by using
cexp().  The ICE you get in PR30969 shows that TARGET_C99_FUNCTIONS is _not_ set:

      /* We can expand via the C99 cexp function.  */
      gcc_assert (TARGET_C99_FUNCTIONS);
Comment 12 Richard Biener 2007-03-13 13:26:11 UTC
Now,

2007-02-05  Roger Sayle  <roger@eyesopen.com>

        * fold-const.c (fold_unary) <REAL_PART>: Test for availability of
        BUILT_IN_COS before simplifying REAL_PART(CEXPI)) to COS.
        <IMAG_PART>: Likewise, check for availability of BUILT_IN_SIN.
        * builtins.c (fold_builtin_sincos): Check for TARGET_C99_FUNCTIONS
        before canonicalizing sincos to cexpi.
        (fold_builtin_cexp): Likewise, for canonicalizing cexp to cexpi.

is too conservative in the sense that we can expand __builtin_cexpi if
either TARGET_HAS_SINCOS or TARGET_C99_FUNCTIONS.
Comment 13 Dominique d'Humieres 2007-03-13 14:48:23 UTC
> The ICE you get in PR30969 shows that TARGET_C99_FUNCTIONS is _not_ set:

Is there a way to test it once the building directory is gone? In gcc/config/rs6000/darwin.h I read:

...
/* Old versions of Mac OS/Darwin don't have C99 functions available.  */
#undef TARGET_C99_FUNCTIONS
#define TARGET_C99_FUNCTIONS                                    \
  (TARGET_64BIT                                                 \
   || strverscmp (darwin_macosx_version_min, "10.3") >= 0)
...

I assumed that TARGET_C99_FUNCTIONS is set for 10.3 and above, but I may have overlooked something else.
Comment 14 Dominique d'Humieres 2007-03-17 00:26:04 UTC
Thanks to Richard Guenther, the bug seems to be fixed (hopefully in the 20070316 snapshot):
see PR31161 for details.
Comment 15 Dominique d'Humieres 2007-03-22 16:22:23 UTC
Note that the drawback of the optimization replacing sin+cos by cexpi on PPC Darwin has been dissected in PR31249.

In comment #16, I proposed a patch. Before applying it, it would be nice to test if the other platforms affected by this bug are also affected by the bad optimization and if the patch does not cause timing regressions on platforms for which the optimization is working (mostly Linux).
Comment 16 Mark Mitchell 2007-06-29 18:00:24 UTC
This problem is reported fixed; may we close this PR?
Comment 17 Paolo Carlini 2007-06-29 18:09:52 UTC
Great, let's close it, then.