Bug 46842 - [4.6 Regression] wrong results with MATMUL(..., TRANSPOSE (func ())) -- 465.tonto test run miscompares
Summary: [4.6 Regression] wrong results with MATMUL(..., TRANSPOSE (func ())) -- 465.t...
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.6.0
: P1 normal
Target Milestone: 4.6.0
Assignee: Not yet assigned to anyone
URL:
Keywords: wrong-code
: 46506 (view as bug list)
Depends on:
Blocks:
 
Reported: 2010-12-07 21:58 UTC by Changpeng Fang
Modified: 2010-12-11 20:35 UTC (History)
7 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2010-12-09 00:04:52


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Changpeng Fang 2010-12-07 21:58:58 UTC
Tests are performed based 4.6 trunk @ 167317 on a linux64 system. 456.tonto test run miscompares @ any options. 

train and ref runs are fine.

LINK: gfortran   -O0  -DSPEC_CPU_LP64        <objects>           -o options
C: LD="gfortran"
O: FOPTIMIZE="-O0"
P: PORTABILITY="-DSPEC_CPU_LP64"
C: LDOUT="-o options"

Build successes: 465.tonto(base)

Setting Up Run Directories
  Setting up 465.tonto test base cfang default: existing (run_base_test_cfang.0000)
Running Benchmarks
  Running 465.tonto test base cfang default
/home/chfang/cpu2006/bin/specinvoke -d /home/chfang/cpu2006/benchspec/CPU2006/465.tonto/run/run_base_test_cfang.0000 -e speccmds.err -o speccmds.stdout -f speccmds.cmd -r -C
/home/chfang/cpu2006/bin/specinvoke -E -d /home/chfang/cpu2006/benchspec/CPU2006/465.tonto/run/run_base_test_cfang.0000 -c 1 -e compare.err -o compare.stdout -f compare.cmd

*** Miscompare of stdout; for details see
    /home/chfang/cpu2006/benchspec/CPU2006/465.tonto/run/run_base_test_cfang.0000/stdout.mis
Error: 1x465.tonto
Comment 1 Tobias Burnus 2010-12-07 22:39:13 UTC
It does not seem to affect all SPEC testers as Rev. 167317 is already quite some old (= 2010-11-30) and I do not have seen any other report. (Cf. for instance http://gcc.gnu.org/ml/gcc-testresults/2010-12/msg00579.html)

I assume it worked before - can you find out which version caused the regression? Or do some other narrowing down of the problem?

Note: To my knowledge none of the gfortran developers has access to SPEC CPU 2006.
Tonto itself is accessible (GPL) from http://www.theochem.uwa.edu.au/tonto/download
Comment 2 Changpeng Fang 2010-12-07 23:12:23 UTC
(In reply to comment #1)
> It does not seem to affect all SPEC testers as Rev. 167317 is already quite
> some old (= 2010-11-30) and I do not have seen any other report. (Cf. for
> instance http://gcc.gnu.org/ml/gcc-testresults/2010-12/msg00579.html)

This bug is filed against test data set. train and ref data sets are fine.

> I assume it worked before - can you find out which version caused the
> regression? Or do some other narrowing down of the problem?
The last working re I observed is 162788 (very very old).
The first bad rev I observed   is 166096 ( very old)
Comment 3 Dominique d'Humieres 2010-12-08 00:21:22 UTC
Could it be a duplicate of PR46506?
Comment 4 Changpeng Fang 2010-12-08 00:50:31 UTC
Yes, a duplicate. A fortran frontend bug?

*** This bug has been marked as a duplicate of bug 46506 ***
Comment 5 Tobias Burnus 2010-12-08 07:02:49 UTC
(In reply to comment #4)
> Yes, a duplicate. A fortran frontend bug?

Maybe - however, in order that we could investigate, it would help if you could narrow down the failing commit - without SPEC CPU 2006 access that's impossible. For the revisions you have given one finds
 58 files changed, 13026 insertions(+), 4788 deletions(-)
Comment 6 Sebastian Pop 2010-12-09 00:04:52 UTC
Reducing.
Comment 7 Sebastian Pop 2010-12-09 00:06:51 UTC
I can confirm that rev165732 miscompiles tonto and rev162788 is working.
This is at -O0.
Comment 8 H.J. Lu 2010-12-09 04:07:04 UTC
*** Bug 46506 has been marked as a duplicate of this bug. ***
Comment 9 H.J. Lu 2010-12-09 04:08:47 UTC
This is caused by revision 164494:

http://gcc.gnu.org/ml/gcc-cvs/2010-09/msg00791.html
Comment 10 H.J. Lu 2010-12-09 05:55:06 UTC
This patch:

http://gcc.gnu.org/ml/gcc-patches/2010-09/msg01621.html

is the cause, which says

---
With this, the transpose optimization is back.
The two previous patches permitted to call gfc_conv_expr_descriptor on transpose's arg, so now we just have to bypass the temporary generation in the transpose case. 
I don't add the testcase from the original commit as there is a wrong code regression introduced by this patch (uncaught by the testsuite) to be fixed in patch 5/5. 
---

But the patch 5/5 doesn't fix tonto.
Comment 11 Tobias Burnus 2010-12-09 11:22:14 UTC
I fear that debugging this will be not easy - especially as to my knowledge none of the gfortran developers has access to SPEC CPU 2006.

 * * *

I tried http://sourceforge.net/projects/tonto-chem/files/tonto/2.3/tonto-2.3.1.tar.bz2/download

If I compile it with a version before the regression, namely
  4.6.0 20100909 (experimental) [trunk revision 164046]
I get only failures with "make tests" - I have to recompile with "-fno-sign-zero" to reduce the failures. However, I still get failures (= differences) to the result of the Intel compiler.

I think one could still try to find differences between such a version and a version after the TRANSPOSE committal.

Compiling notice: You need to remove the "-std=f95" and - to speed up the very slow compile a tiny bit - remove also "-Wall -O". Additionally, as mentioned above, consider using "-fno-sign-zero". (In my case, the configuration is in platforms/GNU-gfortran-on-LINUX.) Unfortunately, almost all changes trigger a complete recompilation - and "make -j5" does not work.

 * * *

I could find TRANSPOSEs in the files mo_localiser.F90, molecule.F90, vec{atom}.F90, marchingcube.F90, isosurface.F90, mat{cpx}.F90, mat3{real}.F90, spacegroup.F90, periodic_fmm_level.F90, cluster.F90, mat{real}.F90, run_mat{real}.F90, molecule.main.F90, unit_cell.F90 and crystal.F90. I tried to create some failing test myself, but they were handled properly.
Comment 12 Tobias Burnus 2010-12-09 13:42:57 UTC
The following is wrongly compiled - however, it is not a regression as it occurs since GCC 4.1. The solution is the same as for gfc_trans_arrayfunc_assign, where the same issue can occur. Cf. PR 44582.

Due to the missing temporary, the array contains 88 88 88 88 instead of 88 1 88 3; the test case works with ifort, NAG and g95.

(There is a slim chance that this is also the problem for Tonto as there was a time window in 4.6 where this optimization was never applied; it should have then also failed with 4.1 to 4.6.)


integer :: a(2,2)
a = reshape([1,2,3,4], [2,2])
call sub(transpose(a))
contains
 subroutine sub(x)
   integer :: x(:,:)
   a(1,:) = 88
   a(2,:) = x(:,1)
   if (    any (a(:,1) /= [88, 1])  &
       .or.any (a(:,2) /= [88, 3])) then
     print *, a
     call abort()
   end if
 end subroutine sub
end
Comment 13 Tobias Burnus 2010-12-09 14:15:13 UTC
Another reason for the Tonto failure could be PR 45777.

I have now looked through the TRANSPOSEs of Tonto (cf. comment 11) - and I did not see anything which looks as if it could go wrong. I think it would help if someone with access to SPEC CPU 2006 could debug this further - and tell which version of Tonto is included in CPU 2006. I have looked the the source of the latest (2.3.1) - maybe the crucial code is an an earlier version.

The issue of comment 12 does not seem to apply to Tonto - but it should be fixed nevertheless.
Comment 14 H.J. Lu 2010-12-09 14:48:46 UTC
crystal.fppized.f90 is micompiled.
Comment 15 Tobias Burnus 2010-12-09 16:40:56 UTC
(In reply to comment #14)
> crystal.fppized.f90 is micompiled.

If I compare 4.6.0 20100909 with today's GCC build for Tonto 2.3.1's crystal.F90, one sees in the original dump that the only TRANSPOSE relevant change is in the line:

       seitz = transpose(self%spacegroup%seitz(:,:,s))

(Older version: Call to _gfortran_transpose_r8, newer one not.) However, when I try to create an example out of it, it works as it should.
Comment 16 Tobias Burnus 2010-12-09 22:50:08 UTC
Thanks to Sebastian for some debugging help.

Reduced test case below.

Older gfortrans and other compilers produce (correct):
   4.0000000       4.0000000       6.0000000       6.0000000
The current 4.6 gfortran produces (wrong):
   3.0000000       3.0000000       7.0000000       7.0000000


implicit none
call sub()
contains
   subroutine sub()
      real, dimension(2,2) :: b
      b = 1.0
      b = matmul(b,transpose(func()))
      print *, b
   end subroutine

   function func() result(res)
      real, dimension(2,2) :: res
      res = reshape([1,2,3,4], [2,2])
   end function
end
Comment 17 Tobias Burnus 2010-12-10 07:26:11 UTC
With the function call - there is no transpose at all:

    // Temporary descriptor
    atmp.10.dim[0].stride = 1;  // Normal bounds: OK
    atmp.10.dim[0].lbound = 0;
    atmp.10.dim[0].ubound = 1;
    atmp.10.dim[1].stride = 2;
    atmp.10.dim[1].lbound = 0;
    atmp.10.dim[1].ubound = 1;
    atmp.10.data = (void * restrict) &A.11;
    atmp.10.offset = 0;
    func (&atmp.10); // Function call - so far OK.
    D.1585 = &atmp.10; // Before this line a TRANSPOSE is missing
    [...]
    _gfortran_matmul_r4 (&atmp.12, D.1580, D.1585, 0, 0, 0B);
Comment 18 Richard Biener 2010-12-10 13:24:08 UTC
As this affects SPEC CPU it should be P1, even though Fortran isn't release
critical (something we might want to change).
Comment 19 Joost VandeVondele 2010-12-10 13:37:32 UTC
(In reply to comment #18)
> Fortran isn't release
> critical (something we might want to change).

yes ... please :-)
Comment 20 Mikael Morin 2010-12-10 21:43:56 UTC
Thanks for reducing Tobias. 
I'm looking right now.
Comment 21 Mikael Morin 2010-12-10 21:57:06 UTC
Does this fix tonto ?

====================
diff --git a/trans-array.c b/trans-array.c
index 52ba831..e312777 100644
--- a/trans-array.c
+++ b/trans-array.c
@@ -5442,6 +5442,10 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
     }
   else if (expr->expr_type == EXPR_FUNCTION)
     {
+      for (n = 0; n < info->dimen; n++)
+	if (info->dim[n] != n)
+	  goto descriptor;
+      
       desc = info->descriptor;
       se->string_length = ss->string_length;
     }
@@ -5460,6 +5464,7 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr * expr, gfc_ss * ss)
       tree to;
       tree base;
 
+descriptor:
       /* Set the string_length for a character array.  */
       if (expr->ts.type == BT_CHARACTER)
 	se->string_length =  gfc_get_expr_charlen (expr);
====================

PS: patch is against ~ 2 months old trunk
Comment 22 Changpeng Fang 2010-12-10 22:20:26 UTC
(In reply to comment #21)
> Does this fix tonto ?

The patch fixed the 465.tonto test miscompare when applied to
the current gcc 4.6 trunk!

Thanks,
Comment 23 Dominique d'Humieres 2010-12-11 13:00:10 UTC
The patch in comment #21 also fixes the test in comment #16, but not the one in comment #12.
Comment 24 Jerry DeLisle 2010-12-11 17:12:38 UTC
The case in 12 is a different bug and not a regression. How about this variation for this PR and get this one closed?

Index: trans-array.c
===================================================================
--- trans-array.c	(revision 167624)
+++ trans-array.c	(working copy)
@@ -5293,6 +5293,16 @@ get_array_charlen (gfc_expr *expr, gfc_se *se)
     }
 }
 
+/* Helper function to check dimensions.  */
+static bool
+dim_ok (gfc_ss_info *info)
+{
+  int n;
+  for (n = 0; n < info->dimen; n++)
+    if (info->dim[n] != n)
+      return false;
+  return true;
+}
 
 /* Convert an array for passing as an actual argument.  Expressions and
    vector subscripts are evaluated and stored in a temporary, which is then
@@ -5588,7 +5598,7 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr *
 
       desc = loop.temp_ss->data.info.descriptor;
     }
-  else if (expr->expr_type == EXPR_FUNCTION)
+  else if (expr->expr_type == EXPR_FUNCTION && dim_ok (info))
     {
       desc = info->descriptor;
       se->string_length = ss->string_length;
Comment 25 Jerry DeLisle 2010-12-11 20:05:26 UTC
Author: jvdelisle
Date: Sat Dec 11 20:05:20 2010
New Revision: 167713

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=167713
Log:
2010-12-11  Mikael Morin   <mikael@gcc.gnu.org>
	    Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/46842
	* trans-array.c (dim_ok): New helper function.
	(gfc_conv_expr_descriptor): Use new helper function to check
	function array is full.

Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-array.c
Comment 26 Jerry DeLisle 2010-12-11 20:10:06 UTC
Author: jvdelisle
Date: Sat Dec 11 20:09:59 2010
New Revision: 167714

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=167714
Log:
2010-12-11  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/46842
	* gfortran.dg/array_function_6.f90: New test.

Added:
    trunk/gcc/testsuite/gfortran.dg/array_function_6.f90
Modified:
    trunk/gcc/testsuite/ChangeLog
Comment 27 Jerry DeLisle 2010-12-11 20:11:31 UTC
Fixed, thanks to Mikael concept.
Comment 28 Jerry DeLisle 2010-12-11 20:35:44 UTC
Closing, see PR46896 for the problem identified in comment 12 here.