Bug 29516 - gfortran miscompiled
Summary: gfortran miscompiled
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: tree-optimization (show other bugs)
Version: 4.2.0
: P3 normal
Target Milestone: 4.2.0
Assignee: Zdenek Dvorak
URL: http://gcc.gnu.org/ml/gcc-patches/200...
Keywords: patch, wrong-code
: 29977 (view as bug list)
Depends on:
Blocks:
 
Reported: 2006-10-19 12:34 UTC by Francois-Xavier Coudert
Modified: 2007-03-03 15:52 UTC (History)
6 users (show)

See Also:
Host:
Target: i386-apple-darwin8.8.1
Build:
Known to work: 4.3.0 4.2.0
Known to fail:
Last reconfirmed: 2007-01-09 12:59:51


Attachments
First part of the tree dumps (828.22 KB, application/octet-stream)
2006-11-25 16:49 UTC, Tobias Schlüter
Details
First part of tree dumps redux (650.51 KB, application/x-gzip)
2006-11-25 16:57 UTC, Tobias Schlüter
Details
A patch (3.68 KB, patch)
2007-01-10 00:04 UTC, Zdenek Dvorak
Details | Diff
A fixed patch. (3.28 KB, patch)
2007-01-10 00:55 UTC, Zdenek Dvorak
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Francois-Xavier Coudert 2006-10-19 12:34:16 UTC
The following program (adapted from our testsuite):

$ cat char_transpose_1.f90 
program main
  implicit none
  integer, parameter :: n1 = 3, n2 = 4, slen = 9
  character (len = slen), dimension (n1, n2) :: a
  integer :: i1, i2

  do i2 = 1, n2
    do i1 = 1, n1
      a (i1, i2) = 'ab'(i1:i1) // 'cde'(i2:i2) // 'cantrip'
    end do
  end do

  call test (transpose (a))
contains
  subroutine test (b)
    character (len = slen), dimension (:, :) :: b

    print *, size (b, 1), "==", n2
    print *, size(b,2), "==", n1

    do i2 = 1, n2
      do i1 = 1, n1
        print *, b (i2, i1), "==", a (i1, i2)
      end do
    end do
  end subroutine test
end program main

gives incorrect results on i386-apple-darwin, with both -m32 and -m64:

$ gfortran char_transpose_1.f90 -g -m32 && ./a.out
           4 ==           4
           4 ==           3
 accantrip==accantrip
 adcantrip==bccantrip
 aecantrip==cccantrip
 adcantrip==adcantrip
 aecantrip==bdcantrip
 aacantrip==cdcantrip
 aecantrip==aecantrip
 aacantrip==becantrip
 ,��l���==cecantrip
 aacantrip==aacantrip
 ,��l���==bacantrip
 �""==cacantrip
$ gfortran char_transpose_1.f90 -m64 && ./a.out
           4 ==           4
           4 ==           3
 accantrip==accantrip
 adcantrip==bccantrip
 aecantrip==cccantrip
 adcantrip==adcantrip
 aecantrip==bdcantrip
 aacantrip==cdcantrip
 aecantrip==aecantrip
 aacantrip==becantrip
 �==cecantrip
 aacantrip==aacantrip
 �==bacantrip
 ���==cacantrip


This is with mainline gfortran:

$ gfortran -v
Using built-in specs.
Target: i386-apple-darwin8.8.1
Configured with: /gcc/configure --enable-languages=c,fortran
Thread model: posix
gcc version 4.2.0 20061019 (experimental)


The corresponding test results can be found here: http://gcc.gnu.org/ml/gcc-testresults/2006-10/msg00952.html (and they don't look too good).
Comment 1 Francois-Xavier Coudert 2006-10-19 22:16:22 UTC
The generated code emitted for the TRANSPOSE for i386-darwin is stupid:

    atmp.13.dtype = parm.12.dtype;
    atmp.13.dim[0].stride = parm.12.dim[1].stride;
    atmp.13.dim[0].lbound = parm.12.dim[1].lbound;
    atmp.13.dim[0].ubound = parm.12.dim[1].ubound;
    atmp.13.dim[1].stride = parm.12.dim[1].stride;
    atmp.13.dim[1].lbound = parm.12.dim[1].lbound;
    atmp.13.dim[1].ubound = parm.12.dim[1].ubound;
    atmp.13.data = parm.12.data;
    atmp.13.offset = parm.12.offset;

when you compare it to the (correct) code emitted on x86-linux:

    atmp.13.dtype = parm.12.dtype;
    atmp.13.dim[0].stride = parm.12.dim[1].stride;
    atmp.13.dim[0].lbound = parm.12.dim[1].lbound;
    atmp.13.dim[0].ubound = parm.12.dim[1].ubound;
    atmp.13.dim[1].stride = parm.12.dim[0].stride;
    atmp.13.dim[1].lbound = parm.12.dim[0].lbound;
    atmp.13.dim[1].ubound = parm.12.dim[0].ubound;
    atmp.13.data = parm.12.data;
    atmp.13.offset = parm.12.offset;
Comment 2 Eric Christopher 2006-10-19 22:24:34 UTC
I'll take a look at this.
Comment 3 Tobias Schlüter 2006-11-25 16:12:12 UTC
*** Bug 29977 has been marked as a duplicate of this bug. ***
Comment 4 Tobias Schlüter 2006-11-25 16:12:44 UTC
The duplicate contains some more analysis.
Comment 5 Tobias Schlüter 2006-11-25 16:20:48 UTC
With the workaround in place, I get a clean -- aside from fallout from another patch in my tree -- testsuite run on i686-darwin
Comment 6 Tobias Schlüter 2006-11-25 16:49:13 UTC
Created attachment 12686 [details]
First part of the tree dumps

At FX' request, I've produced a tarball containing the tree dumps from the compilation of trans-array.c
Comment 7 Tobias Schlüter 2006-11-25 16:57:07 UTC
Created attachment 12687 [details]
First part of tree dumps redux
Comment 8 Tobias Schlüter 2006-11-25 17:46:54 UTC
I've given up attaching the tree dumps, they can be downloaded from
 http://www.cip.physik.uni-muenchen.de/~tobias.schlueter/dumps.tar.gz
Comment 9 Tobias Schlüter 2006-11-25 18:50:32 UTC
The miscompilation appears first with -O -ftree-vrp, i.e. removing -ftree-vrp from the following command line gives a working executable, with it, we get a broken compiler.

tobias-schluters-computer:~/src/trunk/build/gcc tobi$ /Users/tobi/src/trunk/build/./prev-gcc/xgcc -B/Users/tobi/src/trunk/build/./prev-gcc/ -B/Users/tobi/usr/i386-apple-darwin8.8.3/bin/ -c   -O -g -ftree-vrp -fomit-frame-pointer -DIN_GCC   -W -Wall -Wwrite-strings -Wstrict-prototypes -Wmissing-prototypes -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -Wold-style-definition -Wmissing-format-attribute -Werror -fno-common   -DHAVE_CONFIG_H -I. -Ifortran -I../../gcc -I../../gcc/fortran -I../../gcc/../include -I./../intl -I../../gcc/../libcpp/include -I/sw/include  -I../../gcc/../libdecnumber -I../libdecnumber    ../../gcc/fortran/trans-array.c -o fortran/trans-array.o
Comment 10 Francois-Xavier Coudert 2007-01-02 14:46:47 UTC
Hi Eric, any news on that one? It really is a pain, because it's apparently a target-specific middle-end bug that miscompiles gfortran on a platform where people are really starting to use it for production...

Just in case, here's a summary of the problem: in gcc/fortran/trans-array.c (gfc_conv_array_transpose), the following gets miscompiled:

  for (n = 0; n < 2; n++)
    {
      dest_info->delta[n] = gfc_index_zero_node;
      dest_info->start[n] = gfc_index_zero_node;
      dest_info->stride[n] = gfc_index_one_node;
      dest_info->dim[n] = n;

      dest_index = gfc_rank_cst[n];
      src_index = gfc_rank_cst[1 - n]; // This is the miscompiled statement

      /* ... */
    }

When the statement indicated is replaced by, e.g., src_index = gfc_rank_cst[n == 0 ? 1 : 0], everything is OK. It appears that the VRP is responsible for that (see the tree dumps provided by Tobias).
Comment 11 Tobias Schlüter 2007-01-07 19:42:16 UTC
We should maybe install the workaround if the optimizer bug doesn't get fixed soon, as a Fortran FE that produces wrong code for most Fortran 90 codes is probably not something we want.
Comment 12 Richard Biener 2007-01-09 13:14:45 UTC
Do we know why this only fails on darwin?  If it is a VRP bug we should be able to produce a generic testcase.
Comment 13 Richard Biener 2007-01-09 13:33:25 UTC
The problem is definitely the following overflowing mutliplication
which is introduced by ivopts (I'm looking at Tobias dump files):

  D.27347_74 = (union tree_node * *) n_30;
  D.27348_76 = D.27347_74 * 4294967292B;
  src_index_36 = MEM[base: &gfc_rank_cst, index: D.27348_76, offset: 4B];

VRP then (wrongly) asserts

  D.27348_76: [0B, 0B]  EQUIVALENCES: { } (0 elements)

based on (correct)

  D.27347_74: [0B, 4294967295B]  EQUIVALENCES: { } (0 elements)

and removes the index from them MEM:

  D.27347_74 = (union tree_node * *) n_30;
  D.27348_76 = D.27347_74 * 4294967292B;
  src_index_36 = MEM[base: &gfc_rank_cst, offset: 4B];
Comment 14 Richard Biener 2007-01-09 13:42:43 UTC
(which gcc version are the dumps created with?)

First IVOPTs should not create pointer multiplication.  Really.  Second, the
problem is probably in tree-vrp.c:adjust_range_with_scev () or SCEV itself -
I guess SCEV might be confused by this multiplication as well.
Comment 15 Tobias Schlüter 2007-01-09 14:37:38 UTC
(In reply to comment #14)
> (which gcc version are the dumps created with?)

Should be the trunk from 2006-11-25.  Thanks for looking into this.
Comment 16 Tobias Schlüter 2007-01-09 18:21:59 UTC
Dumps from today's mainline (r120620) are at http://www.cip.physik.uni-muenchen.de/~tobias.schlueter/dump2.tar.bz2
Comment 17 Mike Stump 2007-01-09 20:11:19 UTC
Thanks delta:

$ ./xgcc -B./ -c -O t.i -fdump-tree-all &&  grep ' * 4294967292B;' *.087t.ivopts
  D.2035_3 = D.2034_2 * 4294967292B;
$ cat t.i
typedef struct gfc_se { int pre; } gfc_se;
typedef struct gfc_ss_info { int dim[7]; } gfc_ss_info;
int gfc_rank_cst[7 + 1];
gfc_conv_array_transpose (gfc_se * se) {
  int dest, src, dest_index, src_index;
  gfc_ss_info *dest_info;
  int n;
  for (n = 0; n < 2; n++) {
    dest_info->dim[n] = n;
    src_index = gfc_rank_cst[1 - n];
    a (se->pre, b (dest, dest_index), c (src, src_index));
  }
}

this corresponds to the:  D.27348_76 = D.27347_74 * 4294967292B; line above.
Comment 18 Richard Biener 2007-01-09 20:32:30 UTC
So what else is special about darwin?  I built a --enable-targets=all compiler and am using

./cc1 -quiet -O2 t.i -fdump-tree-ivopts -march=nocona -mtune=generic

can you report how you configured gcc and how you are invoking cc1?
Comment 19 Zdenek Dvorak 2007-01-09 20:45:31 UTC
(In reply to comment #14)
> (which gcc version are the dumps created with?)
> 
> First IVOPTs should not create pointer multiplication.  Really.  Second, the
> problem is probably in tree-vrp.c:adjust_range_with_scev () or SCEV itself -
> I guess SCEV might be confused by this multiplication as well.

The problem is really only ivopts, creating pointer multiplication.  I have seen this bug before, after some changes to ivopts, but it is quite hard to reproduce -- one needs to force ivopts to generate pointer offset multiplied by a negative number and ivopts do not do that very often (the circumstances have to be really strange to make this appear useful).

I will prepare a patch.
Comment 20 Mike Stump 2007-01-09 20:46:56 UTC
You have to add -fPIC to see the bug.
Comment 21 Richard Biener 2007-01-09 20:52:11 UTC
Ok, a cross-compiler to i386-apple-darwin8.8.1 and -O -ftree-vrp -fPIC reproduces the bug.  Defering to Zdenek for a fix.
Comment 22 Mike Stump 2007-01-09 23:34:38 UTC
So I'm wondering, does:

Doing diffs in .:
--- ./tree-ssa-address.c.~1~    2006-12-22 21:07:11.000000000 -0800
+++ ./tree-ssa-address.c        2007-01-09 15:30:42.000000000 -0800
@@ -483,7 +483,7 @@ addr_to_parts (aff_tree *addr, tree type
     {
       part = fold_convert (type, addr->elts[i].val);
       if (!double_int_one_p (addr->elts[i].coef))
-       part = fold_build2 (MULT_EXPR, type, part,
+       part = fold_build2 (MULT_EXPR, type, convert (sizetype, part),
                            double_int_to_tree (type, addr->elts[i].coef));
       add_to_parts (parts, type, part);
     }
--------------

fix this bug?  If so, why is that not the right patch?  With it, I get:

gfc_conv_array_transpose (se)
{
  long unsigned int D.1696;
  int * D.1697;
  int * D.1695;
  int * D.1694;
  unsigned int ivtmp.28;
  int n;
  struct gfc_ss_info * dest_info;
  int src_index;
  int dest_index;
  int src;
  int dest;
  int D.1651;
  int D.1650;
  int D.1649;
  int D.1648;

<bb 2>:

  # n_28 = PHI <n_14(4), 0(2)>
<L0>:;
  D.1694_16 = (int *) dest_info_4(D);
  D.1695_27 = (int *) n_28;
  MEM[base: D.1694_16, index: D.1695_27, step: 4B]{dest_info->dim[n]} = n_28;
  D.1696_2 = (long unsigned int) n_28;
  D.1697_3 = D.1696_2 * 4294967292B;
  src_index_6 = MEM[base: &gfc_rank_cst, index: D.1697_3, offset: 4B]{gfc_rank_cst[D.1648]};
  D.1649_8 = c (src_7(D), src_index_6);
  D.1650_11 = b (dest_9(D), dest_index_10(D));
  D.1651_13 = se_12(D)->pre;
  a (D.1651_13, D.1650_11, D.1649_8);
  n_14 = n_28 + 1;
  if (n_14 != 2) goto <L5>; else goto <L2>;

<L5>:;
  goto <bb 3> (<L0>);

<L2>:;
  return;

}
Comment 23 Zdenek Dvorak 2007-01-09 23:49:57 UTC
Subject: Re:  gfortran miscompiled

> So I'm wondering, does:
> 
> Doing diffs in .:
> --- ./tree-ssa-address.c.~1~    2006-12-22 21:07:11.000000000 -0800
> +++ ./tree-ssa-address.c        2007-01-09 15:30:42.000000000 -0800
> @@ -483,7 +483,7 @@ addr_to_parts (aff_tree *addr, tree type
>      {
>        part = fold_convert (type, addr->elts[i].val);
>        if (!double_int_one_p (addr->elts[i].coef))
> -       part = fold_build2 (MULT_EXPR, type, part,
> +       part = fold_build2 (MULT_EXPR, type, convert (sizetype, part),
>                             double_int_to_tree (type, addr->elts[i].coef));
>        add_to_parts (parts, type, part);
>      }
> --------------
> 
> fix this bug?  If so, why is that not the right patch?  With it, I get:

no, it does not, you still have

>   D.1696_2 = (long unsigned int) n_28;
>   D.1697_3 = D.1696_2 * 4294967292B;

this multiplication by a pointer constant.  There are several more
places where such a MULT_EXPR's can be created.
Comment 24 Zdenek Dvorak 2007-01-10 00:04:08 UTC
Created attachment 12875 [details]
A patch

I am testing the attached patch.  It would be great if someone could test it on i386-apple-darwin.
Comment 25 Andrew Pinski 2007-01-10 00:08:03 UTC
For -fPIC testcases, you should do:
/* { dg-do compile { target fpic } } */
Comment 26 Mike Stump 2007-01-10 00:19:41 UTC
Spinng a testsuite run now of Zdenek's patch...
Comment 27 Mike Stump 2007-01-10 00:30:18 UTC
Breaks the build:

../../gcc/gcc/tree-ssa-address.c: In function 'tree_mem_ref_addr':
../../gcc/gcc/tree-ssa-address.c:272: warning: 'addr' is used uninitialized in this function
Comment 28 Mike Stump 2007-01-10 00:48:50 UTC
Testing with:

--- tree-ssa-address.c.~2~      2007-01-09 16:26:28.000000000 -0800
+++ tree-ssa-address.c  2007-01-09 16:34:10.000000000 -0800
@@ -244,7 +244,7 @@
 tree
 tree_mem_ref_addr (tree type, tree mem_ref)
 {
-  tree addr;
+  tree addr = NULL_TREE;
   tree act_elem;
   tree step = TMR_STEP (mem_ref), offset = TMR_OFFSET (mem_ref);
   tree sym = TMR_SYMBOL (mem_ref), base = TMR_BASE (mem_ref);

as it seemed about right.
Comment 29 Zdenek Dvorak 2007-01-10 00:55:10 UTC
Created attachment 12876 [details]
A fixed patch.

Not quite, I forgot to rewrite several occurences of addr to addr_off in the function.  Here is hopefully more correct version of the patch.
Comment 30 Mike Stump 2007-01-10 02:51:04 UTC
Testing looks good:

  http://gcc.gnu.org/ml/gcc-testresults/2007-01/msg00414.html
Comment 31 Zdenek Dvorak 2007-01-11 09:02:29 UTC
Patch: http://gcc.gnu.org/ml/gcc-patches/2007-01/msg00970.html
Comment 32 Zdenek Dvorak 2007-01-12 00:18:04 UTC
Subject: Bug 29516

Author: rakdver
Date: Fri Jan 12 00:17:50 2007
New Revision: 120695

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=120695
Log:
	PR tree-optimization/29516
	* tree-ssa-address.c (tree_mem_ref_addr, add_to_parts,
	most_expensive_mult_to_index, addr_to_parts,
	create_mem_ref, maybe_fold_tmr): Make the type of
	fields of TARGET_MEM_REF sizetype.
	(move_fixed_address_to_symbol, move_pointer_to_base):
	New functions.
	* tree.def (TARGET_MEM_REF): Add comment on types of
	the operands.
	* gcc.dg/tree-ssa/loop-20.c: New test.


Added:
    trunk/gcc/testsuite/gcc.dg/tree-ssa/loop-20.c
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/tree-ssa-address.c
    trunk/gcc/tree.def

Comment 33 Mike Stump 2007-01-17 19:13:01 UTC
I think 4.2 would be a better release with this patch in it, could we push this into 4.2, thanks.  Any concerns about the satefy of the patch?
Comment 34 Jack Howarth 2007-01-17 23:38:21 UTC
Also as the gfortran developers have pointed out, this bug is currently has a target milestone 4.2.0 which implies it was intended to be fixed in gcc 4.2 branch as well. Unfortunately, I am having trouble getting the patch checked into gcc trunk to apply cleanly to gcc 4.2 branch.
Comment 35 Jack Howarth 2007-01-19 03:02:38 UTC
It appears that r118856, r119854 and r120156 be backported for the context of the patch for r120695 to be correct in gcc 4.2 branch.
Comment 36 Zdenek Dvorak 2007-01-23 21:19:09 UTC
Patch for 4.2: http://gcc.gnu.org/ml/gcc-patches/2007-01/msg01941.html
Comment 37 Zdenek Dvorak 2007-01-26 19:56:21 UTC
Subject: Bug 29516

Author: rakdver
Date: Fri Jan 26 19:56:05 2007
New Revision: 121214

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=121214
Log:
	PR tree-optimization/29516
	* tree-ssa-address.c (tree_mem_ref_addr, add_to_parts,
	most_expensive_mult_to_index, addr_to_parts,
	create_mem_ref, maybe_fold_tmr): Make the type of
	fields of TARGET_MEM_REF sizetype.
	(move_fixed_address_to_symbol, move_pointer_to_base,
	aff_combination_remove_elt): New functions.
	* tree.def (TARGET_MEM_REF): Add comment on types of
	the operands.
	* gcc.dg/tree-ssa/loop-20.c: New test.


Added:
    branches/gcc-4_2-branch/gcc/testsuite/gcc.dg/tree-ssa/loop-20.c
Modified:
    branches/gcc-4_2-branch/gcc/ChangeLog
    branches/gcc-4_2-branch/gcc/tree-ssa-address.c
    branches/gcc-4_2-branch/gcc/tree.def

Comment 38 Francois-Xavier Coudert 2007-03-03 15:52:20 UTC
Fixed on 4.3 and 4.2.