User account creation filtered due to spam.

Bug 33759 - ICE in transfer_simplify_4.f90 at any level of optimization
Summary: ICE in transfer_simplify_4.f90 at any level of optimization
Status: RESOLVED FIXED
Alias: None
Product: gcc
Classification: Unclassified
Component: fortran (show other bugs)
Version: 4.3.0
: P3 normal
Target Milestone: 4.4.0
Assignee: Not yet assigned to anyone
URL:
Keywords: ice-on-valid-code, wrong-code
Depends on:
Blocks: 31237 32834
  Show dependency treegraph
 
Reported: 2007-10-12 20:30 UTC by Dominique d'Humieres
Modified: 2008-11-15 04:01 UTC (History)
3 users (show)

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2007-10-23 15:52:17


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Dominique d'Humieres 2007-10-12 20:30:22 UTC
In my comment 31 of PR31608, I have questionned the validity of the test case in comment 28.  Now I think the test is invalid, but not detected. First consider the code:

character(len=20) :: string
character(len=1)  :: tmp(20)
string = ""
tmp = ""
tmp = transfer(string,"x",len(string))
tmp = merge(tmp, string, .true.)
end

gfortran gives:

tmp = merge(tmp, string, .true.)
           1
Error: Unequal character lengths (1 and 20) in MERGE intrinsic at (1)

Note that an error based on the shape would probably better. When using arrays of real, I get:

Error: Different shape for arguments 'tsource' and 'fsource' for intrinsic 'merge' at (1) on dimension 1 (3 and 2)

Now consider the code

  character(len=1)  :: string = "z"
  character(len=20) :: tmp = ""
  tmp = Upper ("abcdefg")
  print *, "'", tmp, "'"
 contains
  Character (len=20) Function Upper (string)
    Character(len=*) string
    character(len=1) :: tmp(20) = ""
    tmp = transfer(string,"x",len(string))
    tmp = merge(tmp, string, .true.)
    Upper = transfer(tmp, "x")
    return
  end function Upper
end

gfortran compiles it without complain and gives

 'a                   '

I think the above code is invalid in two counts:

(1) transfer(string,"x",len(string)) is a rank one array of size 7 and should not be assigned to an array of size 20.  I think this is quite difficult to detect at compile time, but it would be nice to have it at runtime.

(2) merge(tmp, string, .true.) try to merge a rank one character array with a rank zero string. If this invalid and detected in the first test above, it should also be detected in the function, unless I am missing something.
Comment 1 Tobias Burnus 2007-10-12 20:54:18 UTC
> (1) transfer(string,"x",len(string)) is a rank one array of size 7 and should
> not be assigned to an array of size 20.  I think this is quite difficult to
> detect at compile time, but it would be nice to have it at runtime.

NAG f95 agrees with this and prints at run time:
  Rank 1 of array operand has extent 7 instead of 20
  In UPPER, line 9 of test.f90

> (2) merge(tmp, string, .true.) try to merge a rank one character array with a
> rank zero string. If this invalid and detected in the first test above, it
> should also be detected in the function, unless I am missing something.

I think the rank is not a problem as MERGE is an ELEMENTAL function and a scalar ("string") is conformable with any array (such as "tmp"). However, there is the problem that the string length is not the same (1 and 7); NAG f95 prints at run time:

  Differing character lengths (1 and 7) in MERGE
  In UPPER, line 10 of test.f90
Comment 2 Dominique d'Humieres 2007-10-12 22:18:45 UTC
Subject: Re:  Unequal character lengths in MERGE intrinsic
 not detected at run time

> scalar ("string") is conformable with any array (such as "tmp")

Yes, I missed that, so if the length of string is only known at runtime,
it can only be caught at run time, however the Portland Group Fortran
detects it at compile time (constant folding?).

Now I have another odd result: the following code

  character(len=1)  :: string = "z"
  character(len=20) :: tmp = ""
  tmp = Upper ("abcdefgh")
  print *, tmp
 contains
  Character (len=20) Function Upper (string)
    Character(len=*) string
    print *, len(string)
    print *, size(transfer(string,"xy",len(string)))
    Upper = ""
    Upper(1:2) =                                                                &
     transfer(merge(transfer(string,"xy",len(string)),    &
       string(1:2), .true.), "xy")
    return
  end function Upper
end

gives

 8
 8
 ab                  

with xlf, g95, Intel and PGF, but gfortran gives:

           8
           4
 ab                  

a bug or an ambiguity of the transfer definition?
Comment 3 Paul Thomas 2007-10-23 15:52:17 UTC
(In reply to comment #2)
...snip...
> Now I have another odd result: the following code
...snip...
>     print *, size(transfer(string,"xy",len(string)))

I believe that gfortran is incorrect:

In implementing "If the physical representation of the result is longer than that of SOURCE, the physical representation of the leading part is that of SOURCE and the remainder is undefined." I took the number of elements in result to be the physical size of source.

However, "Case (iii): If SIZE is present, the result is array valued of rank one and size SIZE." is nothing if not clear!

I'll figure out how to fix it.

Cheers

Paul
Comment 4 Paul Thomas 2007-10-23 16:08:27 UTC
(In reply to comment #3)
> (In reply to comment #2)

> I'll figure out how to fix it.

This does the job:

Index: gcc/fortran/trans-intrinsic.c
===================================================================
*** gcc/fortran/trans-intrinsic.c	(revision 129504)
--- gcc/fortran/trans-intrinsic.c	(working copy)
*************** gfc_conv_intrinsic_array_transfer (gfc_s
*** 3171,3178 ****
      {
        tmp = fold_build2 (MULT_EXPR, gfc_array_index_type,
  			 tmp, dest_word_len);
-       tmp = fold_build2 (MIN_EXPR, gfc_array_index_type,
- 			 tmp, source_bytes);
      }
    else
      tmp = source_bytes;
--- 3171,3176 ----
*************** gfc_conv_intrinsic_array_transfer (gfc_s
*** 3229,3235 ****
  			 3,
  			 tmp,
  			 fold_convert (pvoid_type_node, source),
! 			 size_bytes);
    gfc_add_expr_to_block (&se->pre, tmp);
  
    se->expr = info->descriptor;
--- 3227,3233 ----
  			 3,
  			 tmp,
  			 fold_convert (pvoid_type_node, source),
! 			 source_bytes);
    gfc_add_expr_to_block (&se->pre, tmp);
  
    se->expr = info->descriptor;

and survives "dg.exp=gfortran/transfer*".  It is going to be a while before I can deal with this, so please feel free.  (I cannot svn commit from where I am.)

Paul
Comment 5 Dominique d'Humieres 2007-10-24 11:36:28 UTC
With the patch in comment #4, the code in #2 yields the same results as other compilers without regression.
Comment 6 Dominique d'Humieres 2007-10-25 09:48:57 UTC
> and survives "dg.exp=gfortran/transfer*".

Let me just point out that gfortran.dg/transfer_simplify_4.f90 fails
with any optimization starting at -O1 due to the failure of the reduced test:

! { dg-do run }
! { dg-options "-O0" }
! Tests that the in-memory representation of a transferred variable
! propagates properly.
!
  implicit none

  integer, parameter :: ip1 = 42
  logical, parameter :: ap1 = transfer(ip1, .true.)
  integer, parameter :: ip2 = transfer(ap1, 0)

  logical :: a
  integer :: i
  
  i = ip1
  a = transfer(i, .true.)
  i = transfer(a, 0)
  print *, i, ip1
  if (i .ne. ip1) call abort ()

end

[karma] f90/bug% gfc -O1 transfer_simplify_4_red.f90
[karma] f90/bug% a.out
           0          42
Abort
Comment 7 Paul Thomas 2007-11-12 15:03:42 UTC
(In reply to comment #6)
> > and survives "dg.exp=gfortran/transfer*".
> Let me just point out that gfortran.dg/transfer_simplify_4.f90 fails
> with any optimization starting at -O1 due to the failure of the reduced test:

Note that this failure occurs without the patch and is a problem with the scalar branch of transfer.  This patch is OK.

A peculiar feature of this failure is that 'i' can be changed to INTEGER(1,2,8) and the test succeeds at any level of optimisation.  It is only at the default logical length that it fails!

Puzzled

Paul
Comment 8 Paul Thomas 2007-11-12 15:34:54 UTC
(In reply to comment #7)

> A peculiar feature of this failure is that 'i' can be changed to INTEGE(1,2,8)
> and the test succeeds at any level of optimisation.  It is only at the default
> logical length that it fails!
> Puzzled

For -O1 and above, .true. == 1_defaultint, whereas it is  .true. == >0_defaultint for no optimization.  Try setting ip1 to 1.

Paul
Comment 9 Dominique d'Humieres 2007-11-12 16:48:43 UTC
Subject: Re:  Unequal character lengths in MERGE intrinsic
 not detected at run time

> This patch is OK.

Yes indeed, I have applied it a long time ago. 

I have only pointed to the last bug on transfer I know of.

> It is only at the default logical length that it fails!

What is the default logical length with -m64? because it fails
only for integer(4) with or without -m64 (PPC Darwin8)
Comment 10 Paul Thomas 2007-11-13 12:46:37 UTC
(In reply to comment #9)

> > This patch is OK.
> Yes indeed, I have applied it a long time ago.

As I found out minutes after I posted this note - thanks!

> I have only pointed to the last bug on transfer I know of.
> > It is only at the default logical length that it fails!

The optimizer is doing something horrible to the code in transfer_simplify_4.f90; I have CC'd Andrew in the hope that he can shed some light on it:

As an experiment, I tried the following patch to circumvent the mashing up of the logical by BUILT_IN_MEMCPY when transferring between LOGICAL and INTEGER:

Index: gcc/fortran/trans-intrinsic.c
===================================================================
*** gcc/fortran/trans-intrinsic.c       (révision 130095)
--- gcc/fortran/trans-intrinsic.c       (copie de travail)
*************** gfc_conv_intrinsic_transfer (gfc_se * se
*** 3277,3282 ****
--- 3277,3291 ----
        se->expr = ptr;
        se->string_length = argse.string_length;
      }
+   else if (expr->value.function.actual->expr->ts.kind == expr->ts.kind
+            && ((expr->value.function.actual->expr->ts.type == BT_LOGICAL
+                       && expr->ts.type == BT_INTEGER)
+            ||  (expr->value.function.actual->expr->ts.type == BT_INTEGER
+                       && expr->ts.type == BT_LOGICAL)))
+     {
+       ptr = convert (build_pointer_type (type), ptr);
+       se->expr = build_fold_indirect_ref (ptr);
+     }
    else
      {
        tree moldsize;

The example in comment #6 now works at any level of optimization, even without the PRINT statement (the reason why I mention this will be apparent in a moment).

transfer_simplify_4.f90 itself fails in the third block, at -O2 and higher, whereas it failed at all levels of optimization withhout the patch.

  a = transfer(ip1, .true.)
  i = transfer(a, 0)
  if (i .ne. ip1) call abort ()

Adding a PRINT *, a; or a PRINT *, i; anywhere in the program allows it to work at any level of optimization.  This is true, even if the PRINT replaces one of the call abort's.

I have looked to see what the i/o might be doing but cannot understand it at all.

Cheers

Paul


Comment 11 Paul Thomas 2007-11-19 07:44:17 UTC
I kept losing this because the title is no longer applicable.

Paul
Comment 12 Paul Thomas 2008-02-26 12:44:20 UTC
Have changed the keyword to represent comment #6

Paul
Comment 13 Francois-Xavier Coudert 2008-03-04 23:47:56 UTC
It will probably be useless to others, who have already dug into this PR, but I did have to research a bit to understand it, so I paste here the results of my research... in case it can help.

-----------------------------------------
For the following reduced code:

  integer :: i
  i = 42
  print *, transfer(i, .true.)
  print *, transfer (transfer(i, .true.), 0)
  end

a.f90.013t.cfg has the (still valid):

  i.6 = i;
  transfer.4 = (logical(kind=4)) i.6;
  transfer.8 = transfer.4;
  D.876 = transfer.8;
  D.882 = D.876;
  transfer.5 = (integer(kind=4)) D.882;
  transfer.9 = transfer.5;
  D.878 = transfer.9;

a.f90.014t.cplxlower0 has made it into a pair of memcpy, but a.f90.015t.veclower has them back in the form of two casts. a.f90.036t.dce1 has them simplified into:

  transfer.4_9 = (logical(kind=4)) i_7;
  transfer.5_11 = (integer(kind=4)) transfer.4_9; 
  D.878 ={v} transfer.5_11;

a.f90.055t.copyrename2 still has that same form:

  transfer.4_9 = (logical(kind=4)) i_7;
  transfer.5_11 = (integer(kind=4)) transfer.4_9;
  D.878 = transfer.5_11;

but a.f90.056t.ccp2 definitely looses it:

  transfer.4_9 = 0;
  transfer.5_11 = 0;
  D.878 = 0;


So, the question is, is cpp2 right to change:

  i_7 = 42;
  transfer.4_9 = (logical(kind=4)) i_7;
  transfer.5_11 = (integer(kind=4)) transfer.4_9;
  D.878 = transfer.5_11;

into

  transfer.4_9 = 0;
  transfer.5_11 = 0;
  D.878 = 0;

I'm not sure what the semantics are for the logical kinds in the middle-end, but it may be that only the least significant bit is used (if you change 42 into 43, you don't get the failure).

According to the related thread at http://groups.google.com/group/comp.lang.fortran/browse_thread/thread/b29957a2f57fec25/27c2b4eca91c511f?lnk=gst&q=transfer+logical#27c2b4eca91c511f correctly (comp.lang.fortran thread named "Transfer and variables that don't use all their storage space.", started by... Tobias Burnus), TRANSFER(TRANSFER(A,B),A) is legal and it's result should always be A (there are some restrictions, but they are fulfilled in this precise case).
-----------------------------------------

Paul, to comment on your patch, it's not only integer and logical that are concerned. You can do the same thing with logical and any other type, I'm sure... and I'm even sure you could manage to do the same thing with a real(kind=10), which has "unused bits" like a real type. (Or maybe, by playing with NaN canonicalization, you could do it with any other floating-point type; ie, choose an integer value that will TRANSFER into a given NaN, and the middle-end might canonicalize this NaN or optimize it in sort weird way.) Also, maybe the optimizer can sometimes be clever enough to see through your added indirection level, can't it?

The only one way I see to do that would be to not create intermediate variables. This is, after all, the meaning of the Fortran standard, that although assignment can normalize (and array transfers are excluded from the TRANSFER(TRANSFER(A,B),A) == A rule), scalar TRANSFERs without assignment don't normalize. 
Comment 14 Paul Thomas 2008-10-21 06:28:40 UTC
This one keeps falling off the pending pile - unassigning myself for now.

Paul
Comment 15 Jakub Jelinek 2008-11-12 17:03:20 UTC
Subject: Bug 33759

Author: jakub
Date: Wed Nov 12 17:01:51 2008
New Revision: 141790

URL: http://gcc.gnu.org/viewcvs?root=gcc&view=rev&rev=141790
Log:
	PR target/35366
	PR fortran/33759
	* fold-const.c (native_encode_string): New function.
	(native_encode_expr): Use it for STRING_CST.

	* trans-const.c (gfc_conv_constant_to_tree): Warn when
	converting an integer outside of LOGICAL's range to
	LOGICAL.
	* trans-intrinsic.c (gfc_conv_intrinsic_function,
	gfc_conv_intrinsic_array_transfer, gfc_conv_intrinsic_transfer):
	Use INTEGER_TYPE instead of BOOLEAN_TYPE for TRANSFER as
	argument of another TRANSFER.

	* gfortran.dg/hollerith.f90: Don't assume a 32-bit value
	stored into logical variable will be preserved.
	* gfortran.dg/transfer_simplify_4.f90: Remove undefined
	cases.  Run at all optimization levels.  Add a couple of
	new tests.
	* gfortran.dg/hollerith5.f90: New test.
	* gfortran.dg/hollerith_legacy.f90: Add dg-warning.

Added:
    trunk/gcc/testsuite/gfortran.dg/hollerith5.f90
Modified:
    trunk/gcc/ChangeLog
    trunk/gcc/fold-const.c
    trunk/gcc/fortran/ChangeLog
    trunk/gcc/fortran/trans-const.c
    trunk/gcc/fortran/trans-intrinsic.c
    trunk/gcc/testsuite/ChangeLog
    trunk/gcc/testsuite/gfortran.dg/hollerith.f90
    trunk/gcc/testsuite/gfortran.dg/hollerith_legacy.f90
    trunk/gcc/testsuite/gfortran.dg/transfer_simplify_4.f90

Comment 16 Paul Thomas 2008-11-13 11:24:17 UTC
Fixed on trunk

Thanks, Jakub

Paul