User account creation filtered due to spam.

Bug 38135 - RESHAPE gives wrong result
Summary: RESHAPE gives wrong result
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.4.0
: P3 normal
Target Milestone: ---
Assignee: Thomas Koenig
URL:
Keywords: wrong-code
Depends on:
Blocks: 32834
  Show dependency treegraph
 
Reported: 2008-11-15 15:56 UTC by Tobias Burnus
Modified: 2008-11-23 15:17 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2008-11-16 09:04:10


Attachments
Patch for the library part (596 bytes, patch)
2008-11-16 16:03 UTC, Thomas Koenig
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tobias Burnus 2008-11-15 15:56:18 UTC
I expect that the following program prints:

    1    2    2
    2    1    2
    2    2    1
    1    0    0    0

which works with g95 and ifort, but with gfortran one gets:
    1    1*****
   72    1    1
*****   72    1
    1    0    0    0

The program is a minutely modified version of the program posted to c.l.f by James Van Buskirk at
http://groups.google.com/group/comp.lang.fortran/browse_thread/thread/e7ec475ff7935804

integer, parameter :: N = 3
integer A(N,N)
A(1:N,1:N)=reshape([integer::],[N,N],reshape([1],[N+1],[2]))
write(*,'(3i5)') A
write(*,'(4i5)') reshape([1],[N+1],[0])
end
Comment 1 Tobias Burnus 2008-11-15 16:10:43 UTC
Using
  A(1:N,1:N)=reshape(A(1:0,1),(/N,N/),reshape((/1/),(/N+1/),(/2/)))
the program is a Fortran 95 program, which also works with NAG f95 and openf95.
-> Blocking 32834
Comment 2 Thomas Koenig 2008-11-15 17:19:00 UTC
$ cat foo.f90
integer, parameter :: N = 3
integer A(N,N)
A(1:N,1:N)=reshape(A(1:0,1),(/N,N/),reshape((/1/),(/N+1/),(/2/)))
write(*,'(3i5)') A
write(*,'(4i5)') reshape([1],[N+1],[0])
end
$ gfortran -fbounds-check foo.f90
$ ./a.out
Fortran runtime error: Incorrect size in SOURCE argument to RESHAPE intrinsic: is 0, should be 9

A false positive for bounds checking as well, it appears.
Comment 3 Thomas Koenig 2008-11-16 09:04:10 UTC
Simplified test case (without the second reshape):

The problem appears to be the empty SOURCE with the presence of PAD.

$ cat bar.f90
program main
  integer, parameter :: N = 3
  integer :: A(N,N)
  integer :: b(n+1)

  b = (/ 1, 2, 2, 2 /)

  A(1:N,1:N)=reshape(A(1:0,1),(/N,N/),b)
  write(*,'(3i5)') A

end program main
$ gfortran bar.f90
$ ./a.out
    1*****    1
*****    1*****
    1*****    1
Comment 4 Mikael Morin 2008-11-16 11:38:16 UTC
(In reply to comment #3)
> The problem appears to be the empty SOURCE with the presence of PAD.
I agree.

There are two bugs actually:
(1) the front-end doesn't expand the reshape. 
    (at least in this case: reshape([integer::],[N,N],[1,2,2,2]) )
(2) reshape gives wrong results. 
Comment 5 Mikael Morin 2008-11-16 13:45:58 UTC
Index: simplify.c
===================================================================
--- simplify.c	(révision 141833)
+++ simplify.c	(copie de travail)
@@ -3410,9 +3410,6 @@ is_constant_array_expr (gfc_expr *e)
   if (e->expr_type != EXPR_ARRAY || !gfc_is_constant_expr (e))
     return false;
   
-  if (e->value.constructor == NULL)
-    return false;
-  
   for (c = e->value.constructor; c; c = c->next)
     if (c->expr->expr_type != EXPR_CONSTANT)
       return false;

This solves (1) (the original testcase), but not (2).
I will regtest it. 

for (2), I'm puzzled. :(
Comment 6 Thomas Koenig 2008-11-16 13:58:23 UTC
(In reply to comment #5)

> 
> for (2), I'm puzzled. :(

I'm onto it; the problems are in reshape.m4 / reshape_generic.c .


Comment 7 Mikael Morin 2008-11-16 15:22:10 UTC
(In reply to comment #6)
> I'm onto it; the problems are in reshape.m4 / reshape_generic.c .
> 
Ok, leaving it to you. 

According to my tests, sstride0 has suspicious values. 
Comment 8 Thomas Koenig 2008-11-16 16:03:45 UTC
Created attachment 16700 [details]
Patch for the library part

Well, here's a patch for the library part.  Apparently, nobody ever paid much attention to the "pad" argument before (a few things were just plain wrong there).

Mikael, you were right about sstride0, but that wasn't the only problem :-)
Comment 9 Mikael Morin 2008-11-16 18:46:41 UTC
(In reply to comment #8)
Are you sure this is needed ?
 
   if (sempty)
     {
-      /* Switch immediately to the pad array.  */
+      /* Pretend we are using the pad array the first time around, too.  */
       src = pptr;
-      sptr = NULL;
+      sptr = pptr;
       sdim = pdim;
       for (dim = 0; dim < pdim; dim++)

sptr is only used later to switch from source to pad, which is not needed here as we start with pad directly. No?


> Mikael, you were right about sstride0, but that wasn't the only problem :-)
While we are at it, we could calculate sstride0 outside the loop, that would be 5ns faster ;-). 

Comment 10 Mikael Morin 2008-11-16 19:44:52 UTC
(In reply to comment #9)
Those are only details, it works nicely :-).

Comment 11 Thomas Koenig 2008-11-16 20:38:01 UTC
Regression-test and full patch, including test cases, tomorrow.

RL keeps interfering :-)
Comment 12 Paul Thomas 2008-11-16 22:25:08 UTC
Thomas,

Since you are there, bar the shouting, I thought that I would assign you:-)

Cheers and thanks

Paul
Comment 13 Thomas Koenig 2008-11-18 22:44:37 UTC
Subject: Bug 38135

Author: tkoenig
Date: Tue Nov 18 22:43:05 2008
New Revision: 141982

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=141982
Log:
2008-11-18  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR libfortran/38135
	* m4/reshape.m4:  Correct bounds checking when PAD is present.
	Treat PAD as if it were SOURCE when SOURCE is empty.
	* intrinsics/reshape_generic.c:  Likewise.
	* generated/reshape_c10.c Regenerated.
	* generated/reshape_c16.c Regenerated.
	* generated/reshape_c4.c Regenerated.
	* generated/reshape_c8.c Regenerated.
	* generated/reshape_i16.c Regenerated.
	* generated/reshape_i4.c Regenerated.
	* generated/reshape_i8.c Regenerated.
	* generated/reshape_r10.c Regenerated.
	* generated/reshape_r16.c Regenerated.
	* generated/reshape_r4.c Regenerated.
	* generated/reshape_r8.c Regenerated.

2008-11-18  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR libfortran/38135
	* gfortran.dg/reshape_pad_1.f90:  New test case.


Added:
    trunk/gcc/testsuite/gfortran.dg/reshape_pad_1.f90
Modified:
    trunk/gcc/testsuite/ChangeLog
    trunk/libgfortran/ChangeLog
    trunk/libgfortran/generated/reshape_c10.c
    trunk/libgfortran/generated/reshape_c16.c
    trunk/libgfortran/generated/reshape_c4.c
    trunk/libgfortran/generated/reshape_c8.c
    trunk/libgfortran/generated/reshape_i16.c
    trunk/libgfortran/generated/reshape_i4.c
    trunk/libgfortran/generated/reshape_i8.c
    trunk/libgfortran/generated/reshape_r10.c
    trunk/libgfortran/generated/reshape_r16.c
    trunk/libgfortran/generated/reshape_r4.c
    trunk/libgfortran/generated/reshape_r8.c
    trunk/libgfortran/intrinsics/reshape_generic.c
    trunk/libgfortran/m4/reshape.m4

Comment 14 Thomas Koenig 2008-11-19 19:33:58 UTC
The problem is fixed on trunk,  I think we can close this
after backporting.

Mikael, if you think the problem you mentioned in comment #4 warrants
its own PR, maybe you could open it.
Comment 15 Mikael Morin 2008-11-19 20:56:03 UTC
(In reply to comment #14)
> Mikael, if you think the problem you mentioned in comment #4 warrants
> its own PR, maybe you could open it.
> 
PR 38184 opened for that.
Comment 16 Thomas Koenig 2008-11-23 15:10:03 UTC
Subject: Bug 38135

Author: tkoenig
Date: Sun Nov 23 15:08:32 2008
New Revision: 142134

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=142134
Log:
2008-11-23  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR libfortran/38135
	Backport from trunk.
	* m4/reshape.m4:  Tread PAD as if it were SOURCE when SOURCE
	is empty.
	* intrinsics/reshape_generic.c:  Likewise.
	* generated/reshape_c10.c Regenerated.
	* generated/reshape_c16.c Regenerated.
	* generated/reshape_c4.c Regenerated.
	* generated/reshape_c8.c Regenerated.
	* generated/reshape_i16.c Regenerated.
	* generated/reshape_i4.c Regenerated.
	* generated/reshape_i8.c Regenerated.
	* generated/reshape_r10.c Regenerated.
	* generated/reshape_r16.c Regenerated.
	* generated/reshape_r4.c Regenerated.
	* generated/reshape_r8.c Regenerated.

2008-11-23  Thomas Koenig  <tkoenig@gcc.gnu.org>

	PR libfortran/38135
	Backport from trunk.
	* gfortran.dg/reshape_pad_1.f90:  New test case.



Added:
    branches/gcc-4_3-branch/gcc/testsuite/gfortran.dg/reshape_pad_1.f90
Modified:
    branches/gcc-4_3-branch/gcc/testsuite/ChangeLog
    branches/gcc-4_3-branch/libgfortran/ChangeLog
    branches/gcc-4_3-branch/libgfortran/generated/reshape_c10.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_c16.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_c4.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_c8.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_i16.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_i4.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_i8.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_r10.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_r16.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_r4.c
    branches/gcc-4_3-branch/libgfortran/generated/reshape_r8.c
    branches/gcc-4_3-branch/libgfortran/intrinsics/reshape_generic.c
    branches/gcc-4_3-branch/libgfortran/m4/reshape.m4

Comment 17 Thomas Koenig 2008-11-23 15:10:30 UTC
Fixed on 4.3.

Closing.
Comment 18 Thomas Koenig 2008-11-23 15:17:27 UTC
... and now really closing.