Bug 69423 - [6 Regression] Invalid optimization with deferred-length character
Summary: [6 Regression] Invalid optimization with deferred-length character
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 6.0
: P4 normal
Target Milestone: 6.0
Assignee: Paul Thomas
URL:
Keywords:
Depends on:
Blocks: 68241
  Show dependency treegraph
 
Reported: 2016-01-21 22:44 UTC by Antony Lewis
Modified: 2016-03-14 18:43 UTC (History)
2 users (show)

See Also:
Host:
Target:
Build:
Known to work: 5.3.0
Known to fail: 6.0
Last reconfirmed: 2016-01-22 00:00:00


Attachments
Clear explicit function result variables for deferred length char arrays (836 bytes, patch)
2016-02-04 14:54 UTC, vehre
Details | Diff
Provisional patch for the PR (2.42 KB, patch)
2016-02-14 14:27 UTC, Paul Thomas
Details | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Antony Lewis 2016-01-21 22:44:43 UTC
Using latest svn master branch, the follow code produces wrong results when compiled with -O1 and higher optimizations:


    program tester
    character(LEN=:), allocatable :: S
    
    S= test(2)
    
    contains
    
        function test(alen)
        character(LEN=:), allocatable :: test
        integer alen, i
    
        do i = alen, 1, -1
                test = 'test'
                exit
        end do
    
        !This line prints nothing when compiled with -O1 and higher
        print *, test
    
        end function test
    
    
    end program tester
Comment 1 Dominique d'Humieres 2016-01-22 10:43:04 UTC
Works with r232023, but not with r232451.
Comment 2 Paul Thomas 2016-01-22 18:14:36 UTC
As the guilty party, I had better take it!

Thanks for the report

Paul
Comment 3 Paul Thomas 2016-02-02 18:18:33 UTC
Dear Anthony,

In reply to your email message, this one is high on my list of PRs to fix. A workaround, which could be permanent, is:

    program tester
    character(LEN=:), allocatable :: S

    S= test(2)
    print *, S

    contains

        function test(alen) result(rtest)
        character(LEN=:), allocatable :: rtest
        integer alen, i

        do i = alen, 1, -1
                rtest = 'test'
                exit
        end do

        !This line prints nothing when compiled with -O1 and higher
        print *, rtest

        end function test


    end program tester

At the moment, I cannot see why an explicit function result should be different. The code displayed with -fdump-tree-original looks to be the same!

I have set things up in previous patches such that the string length is an indirect reference to that in the caller. Apparently, in the original testcase the reference is broken. The memory allocated to 'test' is correct but the string length remains zero. Even weirder is that this is specific to DO blocks....

That said, if Andre can see a way to fix it, I would be more than happy to let him do it :-)

Cheers

Paul
Comment 4 vehre 2016-02-03 10:40:28 UTC
Hi Paul,

I will have a look at it today. May be a fresh pair of eyes can be of help.

- Andre
Comment 5 Paul Thomas 2016-02-03 10:44:30 UTC
(In reply to vehre from comment #4)
> Hi Paul,
> 
> I will have a look at it today. May be a fresh pair of eyes can be of help.
> 
> - Andre

Thanks. I haven't given it much attention yet. I diverted into recursive allocatable derived type components because the patch is already suffering from bit-rot. In preparing it for submission, I uncovered one of the weirdest bugs I have yet encountered - I might have to appeal to the list for help on it :-(

Cheers

Paul
Comment 6 vehre 2016-02-03 17:13:34 UTC
What I have learnt so far:

When the gimple optimisation algorithm 'remove_unused_locals ()' in gcc/tree-ssa-live.c is done, it has removed the temporary for the _gfortran_transfer_character_write's string length. In the previous analysis step this (ssa-)temporary has the uid 3471 in my current fortran. That var occurs in two basicblocks (bb), once in <bb 5> of ssa, where it is only used to check whether a preallocated incoming string has the correct length. That whole bb can be eliminated, because it is unreachable which can be deduced from the propagated attributes. But in the <bb 11> it must not be eliminated as well. Because in both bbs the uid of the temporary is the same, I tried several ways to prevent this association (adding a further temporary using gfc_evaluate_now (), copying the tree using copy_node ()), but to no avail yet. 

Having said this my assumption is, that at both occurrences of .__result_4 the exactly same tree is used (and not a separate copy in each place). Maybe this is the issue, but I am still investigating. Just thought to let you know whether anything of this rings a bell.
Comment 7 Paul Thomas 2016-02-03 19:04:52 UTC
(In reply to vehre from comment #6)
> What I have learnt so far:
> 
> When the gimple optimisation algorithm 'remove_unused_locals ()' in
> gcc/tree-ssa-live.c is done, it has removed the temporary for the
> _gfortran_transfer_character_write's string length. In the previous analysis
> step this (ssa-)temporary has the uid 3471 in my current fortran. That var
> occurs in two basicblocks (bb), once in <bb 5> of ssa, where it is only used
> to check whether a preallocated incoming string has the correct length. That
> whole bb can be eliminated, because it is unreachable which can be deduced
> from the propagated attributes. But in the <bb 11> it must not be eliminated
> as well. Because in both bbs the uid of the temporary is the same, I tried
> several ways to prevent this association (adding a further temporary using
> gfc_evaluate_now (), copying the tree using copy_node ()), but to no avail
> yet. 
> 
> Having said this my assumption is, that at both occurrences of .__result_4
> the exactly same tree is used (and not a separate copy in each place). Maybe
> this is the issue, but I am still investigating. Just thought to let you
> know whether anything of this rings a bell.

Andre,

You have gone further in the diagnosis than I have. However, I would take a look at comment #3. There is no reason at all why gimple should treat explicit and implicit result variables differently. I THINK that the difference must lie in the dead wood that I left in place in trans-decl.c(gfc_get_symbol_decl).

(A few minutes later) I just removed the whole lot and this did not fix this PR. However, unsurprisingly, it does regtest OK, which tells me that the whole lot should go.

I still can see absolutely no difference in the generated code.

Index: /svn/trunk/gcc/fortran/trans-decl.c
===================================================================
*** /svn/trunk/gcc/fortran/trans-decl.c	(revision 233015)
--- /svn/trunk/gcc/fortran/trans-decl.c	(working copy)
*************** gfc_get_symbol_decl (gfc_symbol * sym)
*** 1366,1386 ****
  	}
      }
  
-   /* All deferred character length procedures need to retain the backend
-      decl, which is a pointer to the character length in the caller's
-      namespace and to declare a local character length.  */
-   if (!byref && sym->attr.function
- 	&& sym->ts.type == BT_CHARACTER
- 	&& sym->ts.deferred
- 	&& sym->ts.u.cl->passed_length == NULL
- 	&& sym->ts.u.cl->backend_decl
- 	&& TREE_CODE (sym->ts.u.cl->backend_decl) == PARM_DECL)
-     {
-       sym->ts.u.cl->passed_length = sym->ts.u.cl->backend_decl;
-       gcc_assert (POINTER_TYPE_P (TREE_TYPE (sym->ts.u.cl->passed_length)));
-       sym->ts.u.cl->backend_decl = build_fold_indirect_ref (sym->ts.u.cl->backend_decl);
-     }
- 
    fun_or_res = byref && (sym->attr.result
  			 || (sym->attr.function && sym->ts.deferred));
    if ((sym->attr.dummy && ! sym->attr.function) || fun_or_res)
--- 1366,1371 ----
*************** gfc_get_symbol_decl (gfc_symbol * sym)
*** 1409,1447 ****
  	     (sym->ts.u.cl->passed_length == sym->ts.u.cl->backend_decl))
  	    sym->ts.u.cl->backend_decl = NULL_TREE;
  
- 	  if (sym->ts.deferred && byref)
- 	    {
- 	      /* The string length of a deferred char array is stored in the
- 		 parameter at sym->ts.u.cl->backend_decl as a reference and
- 		 marked as a result.  Exempt this variable from generating a
- 		 temporary for it.  */
- 	      if (sym->attr.result)
- 		{
- 		  /* We need to insert a indirect ref for param decls.  */
- 		  if (sym->ts.u.cl->backend_decl
- 		      && TREE_CODE (sym->ts.u.cl->backend_decl) == PARM_DECL)
- 		    {
- 		      sym->ts.u.cl->passed_length = sym->ts.u.cl->backend_decl;
- 		      sym->ts.u.cl->backend_decl =
- 			build_fold_indirect_ref (sym->ts.u.cl->backend_decl);
- 		    }
- 		}
- 	      /* For all other parameters make sure, that they are copied so
- 		 that the value and any modifications are local to the routine
- 		 by generating a temporary variable.  */
- 	      else if (sym->attr.function
- 		       && sym->ts.u.cl->passed_length == NULL
- 		       && sym->ts.u.cl->backend_decl)
- 		{
- 		  sym->ts.u.cl->passed_length = sym->ts.u.cl->backend_decl;
- 		  if (POINTER_TYPE_P (TREE_TYPE (sym->ts.u.cl->passed_length)))
- 		    sym->ts.u.cl->backend_decl
- 			= build_fold_indirect_ref (sym->ts.u.cl->backend_decl);
- 		  else
- 		    sym->ts.u.cl->backend_decl = NULL_TREE;
- 		}
- 	    }
- 
  	  if (sym->ts.u.cl->backend_decl == NULL_TREE)
  	    length = gfc_create_string_length (sym);
  	  else
--- 1394,1399 ----

Cheers

Paul
Comment 8 vehre 2016-02-04 11:17:34 UTC
I recognize most of the code your patch strips, because I initially wrote/modified it to get the deferred length character arrays working. I am somewhat unconvinced that removing them is safe.
Comment 9 vehre 2016-02-04 14:54:18 UTC
Created attachment 37579 [details]
Clear explicit function result variables for deferred length char arrays

Well, using the explicit result declaration of comment #3 the code is far less optimized. Furthermore are the .__result and __result parameters in the code of comment #3 not cleared in tree-original.

When I add this clearing by modifying trans-decl.c (gfc_trans_deferred_vars (), see attached patch1.txt), i.e.,

__.003t.original:
test (character(kind=1)[1:*.__result] * & __result, integer(kind=4) * .__result, integer(kind=4) & restrict alen)
{
  integer(kind=4) i;

  *.__result = 0;
  *__result = 0B;
...

then I get the same result, like in the original program. The question is, does this information help us in any way?
Comment 10 Paul Thomas 2016-02-14 14:27:43 UTC
Created attachment 37686 [details]
Provisional patch for the PR

I have reworked the handling of deferred string function results to fix this PR. Clearly, using the indirect reference to the passed string length is unsafe; whether an optimization bug or not. It's a pity because it was quite elegant! Instead, I have used the original approach of an in-scope character length, which is nulled on entry and assigned to the result length on exit. Catching all the various cases took quite a lot of rummaging around in trans-decl.c(gfc_trans_deferred_vars)!

The patch is attached. It needs a LOT of cleaning up before submission, which i hope to accomplish in the coming couple of days. However, it regtests OK on the current trunk as it stands. The corresponding deja-gnu-ified testcase is copied below.

Paul

! { dg-do run }
!
! Test the fix for PR69423.
!
! Contributed by Antony Lewis  <antony@cosmologist.info>
!
program tester
  character(LEN=:), allocatable :: S
  S= test(2)
  if (len(S) .ne. 4) call abort
  if (S .ne. "test") call abort
  if (allocated (S)) deallocate (S)

  S= test2(2)
  if (len(S) .ne. 4) call abort
  if (S .ne. "test") call abort
  if (allocated (S)) deallocate (S)
contains
  function test(alen)
    character(LEN=:), allocatable :: test
    integer alen, i
    do i = alen, 1, -1
      test = 'test'
      exit
    end do
!       This line would print nothing when compiled with -O1 and higher.
!       print *, len(test),test
    if (len(test) .ne. 4) call abort
    if (test .ne. "test") call abort
  end function test

  function test2(alen) result (test)
    character(LEN=:), allocatable :: test
    integer alen, i
    do i = alen, 1, -1
      test = 'test'
      exit
    end do
!       This worked before the fix.
!       print *, len(test),test
    if (len(test) .ne. 4) call abort
    if (test .ne. "test") call abort
  end function test2
end program tester
Comment 11 Paul Thomas 2016-02-20 18:27:31 UTC
Author: pault
Date: Sat Feb 20 18:26:59 2016
New Revision: 233589

URL: https://gcc.gnu.org/viewcvs?rev=233589&root=gcc&view=rev
Log:
2016-02-20  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/69423
	* trans-decl.c (create_function_arglist): Deferred character
	length functions, with and without declared results, address
	the passed reference type as '.result' and the local string
	length as '..result'.
	(gfc_null_and_pass_deferred_len): Helper function to null and
	return deferred string lengths, as needed.
	(gfc_trans_deferred_vars): Call it, thereby reducing repeated
	code, add call for deferred arrays and reroute pointer function
	results. Avoid using 'tmp' for anything other that a temporary
	tree by introducing 'type_of_array' for the arrayspec type.

2016-02-20  Paul Thomas  <pault@gcc.gnu.org>

	PR fortran/69423
	* gfortran.dg/deferred_character_15.f90 : New test.


Added:
    trunk/gcc/testsuite/gfortran.dg/deferred_character_15.f90
Modified:
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-decl.c
    trunk/gcc/testsuite/ChangeLog
Comment 12 Paul Thomas 2016-03-09 20:50:30 UTC
Author: pault
Date: Wed Mar  9 20:49:58 2016
New Revision: 234093

URL: https://gcc.gnu.org/viewcvs?rev=234093&root=gcc&view=rev
Log:
2016-03-09  Paul Thomas  <pault@gcc.gnu.org>

	Backport from trunk.
	PR fortran/69423
	* trans-decl.c (create_function_arglist): Deferred character
	length functions, with and without declared results, address
	the passed reference type as '.result' and the local string
	length as '..result'.
	(gfc_null_and_pass_deferred_len): Helper function to null and
	return deferred string lengths, as needed.
	(gfc_trans_deferred_vars): Call it, thereby reducing repeated
	code, add call for deferred arrays and reroute pointer function
	results. Avoid using 'tmp' for anything other that a temporary
	tree by introducing 'type_of_array' for the arrayspec type.

2016-03-09  Paul Thomas  <pault@gcc.gnu.org>

	Backport from trunk.
	PR fortran/64324
	* resolve.c (check_uop_procedure): Prevent deferred length
	characters from being trapped by assumed length error.

	Backport from trunk.
	PR fortran/49630
	PR fortran/54070
	PR fortran/60593
	PR fortran/60795
	PR fortran/61147
	PR fortran/64324
	* trans-array.c (gfc_conv_scalarized_array_ref): Pass decl for
	function as well as variable expressions.
	(gfc_array_init_size): Add 'expr' as an argument. Use this to
	correctly set the descriptor dtype for deferred characters.
	(gfc_array_allocate): Add 'expr' to the call to
	'gfc_array_init_size'.
	* trans.c (gfc_build_array_ref): Expand logic for setting span
	to include indirect references to character lengths.
	* trans-decl.c (gfc_get_symbol_decl): Ensure that deferred
	result char lengths that are PARM_DECLs are indirectly
	referenced both for directly passed and by reference.
	(create_function_arglist): If the length type is a pointer type
	then store the length as the 'passed_length' and make the char
	length an indirect reference to it.
	(gfc_trans_deferred_vars): If a character length has escaped
	being set as an indirect reference, return it via the 'passed
	length'.
	* trans-expr.c (gfc_conv_procedure_call): The length of
	deferred character length results is set TREE_STATIC and set to
	zero.
	(gfc_trans_assignment_1): Do not fix the rse string_length if
	it is a variable, a parameter or an indirect reference. Add the
	code to trap assignment of scalars to unallocated arrays.
	* trans-stmt.c (gfc_trans_allocate): Remove 'def_str_len' and
	all references to it. Instead, replicate the code to obtain a
	explicitly defined string length and provide a value before
	array allocation so that the dtype is correctly set.
	trans-types.c (gfc_get_character_type): If the character length
	is a pointer, use the indirect reference.

2016-03-09  Paul Thomas  <pault@gcc.gnu.org>

	Backport from trunk.
	PR fortran/69423
	* gfortran.dg/deferred_character_15.f90 : New test.

2016-03-09  Paul Thomas  <pault@gcc.gnu.org>

	Backport from trunk.
	PR fortran/49630
	* gfortran.dg/deferred_character_13.f90: New test for the fix
	of comment 3 of the PR.

	Backport from trunk.
	PR fortran/54070
	* gfortran.dg/deferred_character_8.f90: New test
	* gfortran.dg/allocate_error_5.f90: New test

	Backport from trunk.
	PR fortran/60593
	* gfortran.dg/deferred_character_10.f90: New test

	Backport from trunk.
	PR fortran/60795
	* gfortran.dg/deferred_character_14.f90: New test

	Backport from trunk.
	PR fortran/61147
	* gfortran.dg/deferred_character_11.f90: New test

	Backport from trunk.
	PR fortran/64324
	* gfortran.dg/deferred_character_9.f90: New test


Added:
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/allocate_error_5.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_10.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_11.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_12.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_13.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_14.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_15.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_8.f90
    branches/gcc-5-branch/gcc/testsuite/gfortran.dg/deferred_character_9.f90
Modified:
    branches/gcc-5-branch/gcc/fortran/ChangeLog
    branches/gcc-5-branch/gcc/fortran/resolve.c
    branches/gcc-5-branch/gcc/fortran/trans-array.c
    branches/gcc-5-branch/gcc/fortran/trans-decl.c
    branches/gcc-5-branch/gcc/fortran/trans-expr.c
    branches/gcc-5-branch/gcc/fortran/trans-stmt.c
    branches/gcc-5-branch/gcc/fortran/trans-types.c
    branches/gcc-5-branch/gcc/fortran/trans.c
    branches/gcc-5-branch/gcc/testsuite/ChangeLog
Comment 13 Paul Thomas 2016-03-14 18:43:33 UTC
Fixed on trunk and 5-branch

Thanks for the report

Paul