[Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[Paul Thomas]

[-- Attachment #1: Type: text/plain, Size: 1204 bytes --]

:ADDPATCH fortran:

This problem turns out not to be anywhere near as specific as the title 
indicates.  The Bugzilla entry comment #10 points out that b does not 
have to be an allocatable array; any array whose expression has no shape 
will do - eg.an assumed shape array.  Also, any elemental intrinsic will 
do; eg cos instead of conjg.

The error comes about because the scalarizer decides to make the loops 
zero based for this assignment.  Since gfc_conv_array_transpose 
transfers the source offset to the result, regardless of the base for 
the loops, the error is inevitable.  The patch detects zero based loops 
and sets the offset to zero.  As far as I can tell, this is the only 
array intrinsic that has this behaviour.  The testscase is based on the 
reporter's, although I used real(8) and cos(), instead of complex(8) and 
conjg().

Regtested on amd64/Cygwin_NT - OK for trunk?

Paul

2007-08-12  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/32962
    * trans-array.c (gfc_conv_array_transpose): Set the offset
    of the destination to zero if the loop is zero based.

2007-08-12  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/32962
    * gfortran.dg/transpose_1.f90: New test.


[-- Attachment #2: pr32962.diff --]
[-- Type: text/x-patch, Size: 3437 bytes --]

Index: gcc/fortran/trans-array.c
===================================================================
*** gcc/fortran/trans-array.c	(revision 127212)
--- gcc/fortran/trans-array.c	(working copy)
*************** gfc_conv_array_transpose (gfc_se * se, g
*** 783,795 ****
    dest_info->data = gfc_conv_descriptor_data_get (src);
    gfc_conv_descriptor_data_set (&se->pre, dest, dest_info->data);
  
!   /* Copy the offset.  This is not changed by transposition: the top-left
!      element is still at the same offset as before.  */
!   dest_info->offset = gfc_conv_descriptor_offset (src);
    gfc_add_modify_expr (&se->pre,
  		       gfc_conv_descriptor_offset (dest),
  		       dest_info->offset);
! 
    if (dest_info->dimen > loop->temp_dim)
      loop->temp_dim = dest_info->dimen;
  }
--- 783,800 ----
    dest_info->data = gfc_conv_descriptor_data_get (src);
    gfc_conv_descriptor_data_set (&se->pre, dest, dest_info->data);
  
!   /* Copy the offset.  This is not changed by transposition; the top-left
!      element is still at the same offset as before, except where the loop
!      starts at zero.  */
!   if (!integer_zerop (loop->from[0]))
!     dest_info->offset = gfc_conv_descriptor_offset (src);
!   else
!     dest_info->offset = gfc_index_zero_node;
! 
    gfc_add_modify_expr (&se->pre,
  		       gfc_conv_descriptor_offset (dest),
  		       dest_info->offset);
! 	  
    if (dest_info->dimen > loop->temp_dim)
      loop->temp_dim = dest_info->dimen;
  }
*************** gfc_conv_loop_setup (gfc_loopinfo * loop
*** 3220,3226 ****
  	  /* Criteria for choosing a loop specifier (most important first):
  	     doesn't need realloc
  	     stride of one
! 	     known stride
  	     known lower bound
  	     known upper bound
  	   */
--- 3225,3231 ----
  	  /* Criteria for choosing a loop specifier (most important first):
  	     doesn't need realloc
  	     stride of one
! 	     known stride (preferably a variable)
  	     known lower bound
  	     known upper bound
  	   */
*************** gfc_conv_loop_setup (gfc_loopinfo * loop
*** 3232,3237 ****
--- 3237,3248 ----
  	  else if (INTEGER_CST_P (info->stride[n])
  		   && !INTEGER_CST_P (specinfo->stride[n]))
  	    loopspec[n] = ss;
+ #if 0
+ 	  else if (INTEGER_CST_P (info->stride[n])
+ 		   && ss->type != EXPR_FUNCTION
+ 		   && loopspec[n]->expr->expr_type == EXPR_FUNCTION)
+ 	    loopspec[n] = ss;
+ #endif
  	  else if (INTEGER_CST_P (info->start[n])
  		   && !INTEGER_CST_P (specinfo->start[n]))
  	    loopspec[n] = ss;
Index: gcc/testsuite/gfortran.dg/transpose_1.f90
===================================================================
*** gcc/testsuite/gfortran.dg/transpose_1.f90	(revision 0)
--- gcc/testsuite/gfortran.dg/transpose_1.f90	(revision 0)
***************
*** 0 ****
--- 1,27 ----
+ ! { dg-do compile }
+ ! Tests the fix for PR32962, in which the result of TRANSPOSE, when
+ ! an actual argument of an elemental intrinsic would receive the
+ ! wrong offset.
+ !
+ ! Contributed by Wirawan Purwanto <wirawan0@gmail.com>
+ !
+   real(kind=8), allocatable :: b(:,:)
+   real(kind=8) :: a(2,2), c(2,2)
+   i = 2
+   allocate (b(i,i))
+   a(1,1) = 2
+   a(2,1) = 3
+   a(1,2) = 7
+   a(2,2) = 11
+   call foo
+   call bar
+   if (any (c .ne. b)) call abort
+ contains
+   subroutine foo
+     b = cos(transpose(a))
+   end subroutine
+   subroutine bar
+     c = transpose(a)
+     c = cos(c)
+   end subroutine
+ end program

Re: [Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[FX Coudert]

:REVIEWMAIL:

> 2007-08-12  Paul Thomas  <pault@gcc.gnu.org>
>
>    PR fortran/32962
>    * trans-array.c (gfc_conv_array_transpose): Set the offset
>    of the destination to zero if the loop is zero based.

OK, except for:

> *************** gfc_conv_loop_setup (gfc_loopinfo * loop
> *** 3220,3226 ****
>   	  /* Criteria for choosing a loop specifier (most important first):
>   	     doesn't need realloc
>   	     stride of one
> ! 	     known stride
>   	     known lower bound
>   	     known upper bound
>   	   */
> --- 3225,3231 ----
>   	  /* Criteria for choosing a loop specifier (most important first):
>   	     doesn't need realloc
>   	     stride of one
> ! 	     known stride (preferably a variable)
>   	     known lower bound
>   	     known upper bound
>   	   */

Do you want this, or is this, like the following, part of another patch?

> *************** gfc_conv_loop_setup (gfc_loopinfo * loop
> *** 3232,3237 ****
> --- 3237,3248 ----
>   	  else if (INTEGER_CST_P (info->stride[n])
>   		   && !INTEGER_CST_P (specinfo->stride[n]))
>   	    loopspec[n] = ss;
> + #if 0
> + 	  else if (INTEGER_CST_P (info->stride[n])
> + 		   && ss->type != EXPR_FUNCTION
> + 		   && loopspec[n]->expr->expr_type == EXPR_FUNCTION)
> + 	    loopspec[n] = ss;
> + #endif
>   	  else if (INTEGER_CST_P (info->start[n])
>   		   && !INTEGER_CST_P (specinfo->start[n]))
>   	    loopspec[n] = ss;


Thanks!
FX

Re: [Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[Paul Thomas]

FX,
> :REVIEWMAIL:
>
> OK, except for:
>
>> *************** gfc_conv_loop_setup (gfc_loopinfo * loop
>> *** 3220,3226 ****
>>         /* Criteria for choosing a loop specifier (most important 
>> first):
>>            doesn't need realloc
>>            stride of one
>> !          known stride
>>            known lower bound
>>            known upper bound
>>          */
>> --- 3225,3231 ----
>>         /* Criteria for choosing a loop specifier (most important 
>> first):
>>            doesn't need realloc
>>            stride of one
>> !          known stride (preferably a variable)
>>            known lower bound
>>            known upper bound
>>          */
>
> Do you want this, or is this, like the following, part of another patch?
>
Ah!  That comment pertained to the version posted to the PR - thanks for 
pointing it out.

Cheers

Paul

Re: [Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[Tobias Burnus]

Paul Thomas wrote:
> Regtested on amd64/Cygwin_NT - OK for trunk?
I wonder whether it makes sense to backport it given that this is a
nasty wrong-code bug and the change is very localized; however, on the
basis that it is not a regression, I do not mind if we don't.

Tobias

Re: [Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[Paul Thomas]

Tobias,
> Paul Thomas wrote:
>   
>> Regtested on amd64/Cygwin_NT - OK for trunk?
>>     
> I wonder whether it makes sense to backport it given that this is a
> nasty wrong-code bug and the change is very localized; however, on the
> basis that it is not a regression, I do not mind if we don't.
>   
I'd prefer not to but.... well, I'll check out the state of my gcc-4.2 
tree :-)

What does everybody else think?

Paul

Re: [Patch, fortran] PR32962 - b = conjg(transpose(a)) is erroneous if b is an allocatable array

[François-Xavier Coudert]

> I'd prefer not to but.... well, I'll check out the state of my gcc-4.2
> tree :-)

I wouldn't bother doing it. It's not a regression, and we should
concentrate our efforts on 4.3.

FX