[Bug fortran/46896] New: Wrong code with transpose(a) passed to subroutine

[Bug fortran/46896] New: Wrong code with transpose(a) passed to subroutine

[jvdelisle at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

           Summary: Wrong code with transpose(a) passed to subroutine
           Product: gcc
           Version: 4.6.0
            Status: UNCONFIRMED
          Keywords: wrong-code
          Severity: major
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: jvdelisle@gcc.gnu.org


This test case is from comment 12 of pr46842 and is a new bug.

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
   print *, a
 end subroutine sub
end

Gives:

$ ./a.out 
          88          88          88          88

Should give:

$ ./a.out 
          88           1          88           3

This is a serious bug and should get some priority.

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4/6 Regression] Wrong code with transpose(a) passed to subroutine

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2010.12.11 23:52:35
     Ever Confirmed|0                           |1

--- Comment #2 from H.J. Lu <hjl.tools at gmail dot com> 2010-12-11 23:52:35 UTC ---
The bad patch was posted at

http://gcc.gnu.org/ml/gcc-patches/2005-12/msg00437.html

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4/6 Regression] Wrong code with transpose(a) passed to subroutine

[hjl.tools at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

H.J. Lu <hjl.tools at gmail dot com> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |hjl.tools at gmail dot com,
                   |                            |rsandifo at gcc dot gnu.org
   Target Milestone|---                         |4.6.0
            Summary|Wrong code with             |[4.2/4.3/4.4/4.5/4/6
                   |transpose(a) passed to      |Regression] Wrong code with
                   |subroutine                  |transpose(a) passed to
                   |                            |subroutine

--- Comment #1 from H.J. Lu <hjl.tools at gmail dot com> 2010-12-11 23:51:51 UTC ---
This is caused by revision 108459:

http://gcc.gnu.org/ml/gcc-patches/2005-12/msg00437.html

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4/6 Regression] Wrong code with transpose(a) passed to subroutine

[jvdelisle at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Jerry DeLisle <jvdelisle at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|4.6.0                       |---

--- Comment #3 from Jerry DeLisle <jvdelisle at gcc dot gnu.org> 2010-12-12 00:38:53 UTC ---
This preliminary patch fixes it.

Index: trans-array.c
===================================================================
--- trans-array.c    (revision 167715)
+++ trans-array.c    (working copy)
@@ -5419,17 +5419,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr *

     case EXPR_FUNCTION:

-      /* We don't need to copy data in some cases.  */
-      arg = gfc_get_noncopying_intrinsic_argument (expr);
-      if (arg)
-    {
-      /* This is a call to transpose...  */
-      gcc_assert (expr->value.function.isym->id == GFC_ISYM_TRANSPOSE);
-      /* ... which has already been handled by the scalarizer, so
-         that we just need to get its argument's descriptor.  */
-      gfc_conv_expr_descriptor (se, expr->value.function.actual->expr, ss);
-      return;
-    }

       /* A transformational function return value will be a temporary
      array descriptor.  We still need to go through the scalarizer
@@ -5452,14 +5441,6 @@ gfc_conv_expr_descriptor (gfc_se * se, gfc_expr *

       if (ss->expr != expr || ss->type != GFC_SS_FUNCTION)
     {
-      if (ss->expr != expr)
-        /* Elemental function.  */
-        gcc_assert ((expr->value.function.esym != NULL
-             && expr->value.function.esym->attr.elemental)
-            || (expr->value.function.isym != NULL
-                && expr->value.function.isym->elemental));
-      else
-        gcc_assert (ss->type == GFC_SS_INTRINSIC);

       need_tmp = 1;
       if (expr->ts.type == BT_CHARACTER

The first hunk gets rid of the bypassing where it is assumed the scalarizer
handled it.

The second hunk just lets compilation continue by removing the asserts which
assume function attributes.

I am not suggesting this is the right way to fix this.  It just identified the
general area that needs to be refined.

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |burnus at gcc dot gnu.org

--- Comment #4 from Tobias Burnus <burnus at gcc dot gnu.org> 2010-12-12 09:01:36 UTC ---
Copy quote from bug 468742 comment 12:

The solution is the same as for gfc_trans_arrayfunc_assign, where the same
issue can occur. Cf. PR 44582.

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #5 from Mikael Morin <mikael at gcc dot gnu.org> 2010-12-12 14:58:18 UTC ---
Created attachment 22725
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22725
Starting point patch

This passes gfortran.dg/*transpose*

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Mikael Morin <mikael at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |mikael at gcc dot gnu.org

--- Comment #6 from Mikael Morin <mikael at gcc dot gnu.org> 2010-12-12 15:04:03 UTC ---
For the cases to be discarded from optimization, the patch above reuses
pr44582's code blindly, hoping it's correct.

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #7 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2010-12-12 16:37:48 UTC ---
The patch in comment #5 fixes this PR, but the tests in pr41278, pr44912, and
the variant below of pr40646 yields ICEs:

"in proc_call_can_redefine_sym, at fortran/trans-expr.c:5411" for (variant of
pr40646 and pr44912)
or "pr41278.f90:10:0: internal compiler error: Segmentation fault".

! { dg-do run }
!
! PR 40646
!
! array-valued PPCs
!
! Original test case by Charlie Sharpsteen <chuck@sharpsteen.net>
! Modified by Janus Weil <janus@gcc.gnu.org>

module bugTestMod
  implicit none
  type:: boundTest
    procedure(returnMat), pointer, nopass:: test
  end type boundTest
contains
  function returnMat( a, b ) result( mat )
    integer:: a, b
    double precision, dimension(a,b):: mat 
    mat = 1d0
  end function returnMat
end module bugTestMod

program bugTest
  use bugTestMod
  implicit none
  type( boundTest ):: testObj
  double precision, dimension(2,2):: testCatch
  testObj%test => returnMat
  testCatch = testObj%test(2,2)
  print *,testCatch
  if (sum(testCatch)/=4) call abort()
end program bugTest

! { dg-final { cleanup-modules "bugTestMod" } }

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[dominiq at lps dot ens.fr]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #8 from Dominique d'Humieres <dominiq at lps dot ens.fr> 2010-12-12 20:47:48 UTC ---
Not surprisingly with the patch in comment #5 I get the following regressions:


FAIL: gfortran.dg/array_function_5.f90  *  (internal compiler error)
FAIL: gfortran.dg/proc_ptr_comp_9.f90  *  (internal compiler error)
FAIL: gfortran.dg/proc_ptr_comp_12.f90  *  (internal compiler error)

The backtrace for gfortran.dg/array_function_5.f90 is

Program received signal EXC_BAD_ACCESS, Could not access memory.
Reason: KERN_INVALID_ADDRESS at address: 0x0000000000000028
proc_call_can_redefine_sym (proc=0x14191f000) at
../../work/gcc/fortran/trans-expr.c:5409
5409      gfc_symbol *sym = expr->symtree->n.sym;
(gdb) bt
#0  proc_call_can_redefine_sym (proc=0x14191f000) at
../../work/gcc/fortran/trans-expr.c:5409
#1  0x00000001000da15e in gfc_conv_procedure_call (se=0x7fff5fbfd500,
sym=0x14191f000, args=0x1419182b0, expr=0x1419181f0, append_args=0x141f1f380)
at ../../work/gcc/fortran/trans-expr.c:3114
Previous frame inner to this frame (gdb could not unwind past this frame)

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Paul Thomas <pault at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         AssignedTo|unassigned at gcc dot       |pault at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #9 from Paul Thomas <pault at gcc dot gnu.org> 2010-12-13 12:04:09 UTC ---
Created attachment 22739
  --> http://gcc.gnu.org/bugzilla/attachment.cgi?id=22739
A tentative fix for the PR

The attached fixes the PR and is regtesting right now.  I do not believe that
there will be any ere regressions since the patch adds temporaries where there
were none before.

However, I will check for consistency with gfc_trans_arrayfunc_assign, as
Tobias suggests.

Cheers

Paul

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #10 from Paul Thomas <pault at gcc dot gnu.org> 2010-12-13 12:06:52 UTC ---
(In reply to comment #9)

I spoke too soon :-(

> I do not believe that
> there will be any ere regressions since the patch adds temporaries where there
> were none before.
FAIL: gfortran.dg/inline_transpose_1.f90  -Os  (test for excess errors)
FAIL: gfortran.dg/inline_transpose_1.f90  -Os  scan-tree-dump-times original
"struct[^\n]*atmp" 24

However, it is probably trivial.

Cheers

Paul

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[mikael at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #11 from Mikael Morin <mikael at gcc dot gnu.org> 2010-12-13 21:24:35 UTC ---
(In reply to comment #9)
> Created attachment 22739 [details]
> A tentative fix for the PR
Yeah, trying to share the code with gfc_trans_arrayfunc_assign was plain pain.
Copy'n'paste will work better here. :-(

> 
> The attached fixes the PR and is regtesting right now.  I do not believe that
> there will be any ere regressions since the patch adds temporaries where there
> were none before.
... unless the testcases check for the absence of temporary ;-)


I won't dig into it further, sorry.

[Bug fortran/46896] [4.2/4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #12 from Paul Thomas <pault at gcc dot gnu.org> 2010-12-14 05:35:00 UTC ---
(In reply to comment #11)
> (In reply to comment #9)

> > The attached fixes the PR and is regtesting right now.  I do not believe that
> > there will be any ere regressions since the patch adds temporaries where there
> > were none before.
> ... unless the testcases check for the absence of temporary ;-)

That's what I meant by trivial. I have to sift through the extra temporaries
and decide which ones are warranted and which ones are not.

Thanks

Paul

[Bug fortran/46896] [4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[rguenth at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Richard Guenther <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |4.3.6
            Summary|[4.2/4.3/4.4/4.5/4.6        |[4.3/4.4/4.5/4.6
                   |Regression] Wrong code with |Regression] Wrong code with
                   |transpose(a) passed to      |transpose(a) passed to
                   |subroutine                  |subroutine

[Bug fortran/46896] [4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[jakub at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Jakub Jelinek <jakub at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P4
                 CC|                            |jakub at gcc dot gnu.org

[Bug fortran/46896] [4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #13 from Paul Thomas <pault at gcc dot gnu.org> 2011-01-08 19:17:06 UTC ---
Author: pault
Date: Sat Jan  8 19:17:03 2011
New Revision: 168600

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=168600
Log:
2011-01-08  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/46896
    * trans-expr.c (gfc_conv_procedure_call): With a non-copying
    procedure argument (eg TRANSPOSE) use a temporary if there is
    any chance of aliasing due to host or use association.
    (arrayfunc_assign_needs_temporary): Correct logic for function
    results and do not use a temporary for implicitly PURE
    variables.  Use a temporary for Cray pointees.
    * symbol.c (gfc_add_save): Explicit SAVE not compatible with
    implicit pureness of containing procedure.
    * decl.c (match_old_style_init, gfc_match_data): Where decl
    would fail in PURE procedure, set implicit_pure to zero.
    * gfortran.h : Add implicit_pure to structure symbol_attr and
    add prototype for function gfc_implicit_pure.
    * expr.c (gfc_check_pointer_assign, gfc_check_vardef_context):
    Where decl would fail in PURE procedure, reset implicit_pure.
    * io.c (match_vtag, gfc_match_open, gfc_match_close,
    gfc_match_print, gfc_match_inquire, gfc_match_wait): The same.
    * match.c (gfc_match_critical, gfc_match_stopcode,
    sync_statement, gfc_match_allocate, gfc_match_deallocate): The
    same.
    * parse.c (decode_omp_directive): The same.
    (parse_contained): If not PURE, set implicit pure attribute.
    * resolve.c (resolve_formal_arglist, resolve_structure_cons,
    resolve_function, resolve_ordinary_assign) : The same.
    (gfc_implicit_pure): New function.
    * module.c (mio_symbol_attribute): Introduce AB_IMPLICIT_PURE
    to ab_attribute enum and use it in this function.

2011-01-08  Paul Thomas  <pault@gcc.gnu.org>

    PR fortran/46896
    * gfortran.dg/transpose_optimization_2.f90 : New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/transpose_optimization_2.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/decl.c
    trunk/gcc/fortran/expr.c
    trunk/gcc/fortran/gfortran.h
    trunk/gcc/fortran/io.c
    trunk/gcc/fortran/match.c
    trunk/gcc/fortran/module.c
    trunk/gcc/fortran/parse.c
    trunk/gcc/fortran/resolve.c
    trunk/gcc/fortran/symbol.c
    trunk/gcc/fortran/trans-expr.c
    trunk/gcc/testsuite/ChangeLog

[Bug fortran/46896] [4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[burnus at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

Tobias Burnus <burnus at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |FIXED
   Target Milestone|4.3.6                       |4.6.0
            Summary|[4.3/4.4/4.5 Regression]    |[4.3/4.4/4.5/4.6
                   |Wrong code with             |Regression] Wrong code with
                   |transpose(a) passed to      |transpose(a) passed to
                   |subroutine                  |subroutine

--- Comment #14 from Tobias Burnus <burnus at gcc dot gnu.org> 2011-02-14 09:22:06 UTC ---
Close PR - won't fix for 4.3 to 4.5, FIXED for 4.6.

Reasoning for closing the PR without backporting:
- Not backported so far
- Rather large patch
- Small patch possible, but with significant performance issues
- Old bug (since December 2005) and not reported for real-world code;
  I think it is unlikely to be hit in real code as the example
  is rather constructed*

* The bug only occurs if a subroutine accesses both a TRANSPOSEd variable,
which is passed as actual argument, and the same variable via host or use
association. In that case a temporary is needed for the actual argument.


Feel free to re-open if you think it really should be backported.

[Bug fortran/46896] [4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[paul.richard.thomas at gmail dot com]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #15 from paul.richard.thomas at gmail dot com <paul.richard.thomas at gmail dot com> 2011-02-14 13:08:13 UTC ---
Tobias,

You shame me - I undertook to do something with this one and did not.
I do not know that the fix is compatible even with 4.5 - I think not.
I will take a quick look tonight.


> Feel free to re-open if you think it really should be backported.

Better still, backport if it is possible :-)

Thanks

Paul

[Bug fortran/46896] [4.3/4.4/4.5/4.6 Regression] Wrong code with transpose(a) passed to subroutine

[pault at gcc dot gnu.org]

http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46896

--- Comment #16 from Paul Thomas <pault at gcc dot gnu.org> 2011-03-13 14:16:25 UTC ---
(In reply to comment #15)
> Tobias,
> 
> You shame me - I undertook to do something with this one and did not.
> I do not know that the fix is compatible even with 4.5 - I think not.
> I will take a quick look tonight.
> 
> 
> > Feel free to re-open if you think it really should be backported.
> 
> Better still, backport if it is possible :-)
> 
> Thanks
> 
> Paul

15 out of 37 chunks failed to apply to 4.5.  They point to all sorts of
divergences between trunk and 4.5 that I do not have the time to deal with.

I think that the answer is that it is not possible to backport.

Paul