[Bug fortran/55501] New: ICE using MERGE in constant expr

[Bug fortran/55501] New: ICE using MERGE in constant expr

[longb at cray dot com]

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

             Bug #: 55501
           Summary: ICE using MERGE in constant expr
    Classification: Unclassified
           Product: gcc
           Version: 4.7.2
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: fortran
        AssignedTo: unassigned@gcc.gnu.org
        ReportedBy: longb@cray.com


Test (extracted from a larger module)

> cat test2.f90
module test

integer,parameter,private :: int32 = 4
integer,private,parameter :: dik = kind(0)     ! Default Integer Kind

type MPI_Datatype
   integer :: mpi_val
end type

type(MPI_Datatype),parameter,private :: MPIx_I4       = MPI_Datatype(4)
type(MPI_Datatype),parameter,private :: MPIx_I8       = MPI_Datatype(8)
type(MPI_Datatype),parameter :: MPI_INTEGER           = merge(MPIx_I4, MPIx_I8,
 dik==int32)

end module test

> ftn -c test2.f90
f951: internal compiler error: in gfc_conv_structure, at
fortran/trans-expr.c:5360
Please submit a full bug report,
with preprocessed source if appropriate.
See <http://gcc.gnu.org/bugs.html> for instructions.

Also fails with 4.8.

Allowing MERGE here is a F2003 feature (elemental intrinsic with constant
arguments). In any case, the compiler should not abort.

Compiles OK with Cray compiler.

[Bug fortran/55501] ICE using MERGE in constant expr

[longb at cray dot com]

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

--- Comment #1 from Bill Long <longb at cray dot com> 2012-11-28 00:37:14 UTC ---
Plain integers in MERGE seem to be OK, so the problem appears to be with using
constants of derived type as arguments to MERGE here.

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

janus at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Keywords|                            |ice-on-valid-code
             Status|UNCONFIRMED                 |NEW
   Last reconfirmed|                            |2012-11-28
                 CC|                            |janus at gcc dot gnu.org
            Summary|ICE using MERGE in constant |[F03] ICE using MERGE in
                   |expr                        |constant expr
     Ever Confirmed|0                           |1

--- Comment #2 from janus at gcc dot gnu.org 2012-11-28 09:16:13 UTC ---
I can confirm this error. It occurs with all gfortran versions I tried
(starting from 4.3 up to the current trunk). Slightly compactified test case:

module test
  type MPI_Datatype
    integer :: mpi_val
  end type
  type(MPI_Datatype) :: MPI_INTEGER = merge(MPI_Datatype(4), MPI_Datatype(8),
.false.)
end module 

Apparently the declaration needs to be in a module for the error to occur, and
also derived types need to be involved.


Using -std=f95, the test case is correctly rejected (without ICE):

type(MPI_Datatype) :: MPI_INTEGER = merge(MPI_Datatype(4), MPI_Datatype(8), .
                                     1
Error: Fortran 2003: Elemental function as initialization expression with
non-integer/non-character arguments at (1)

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

--- Comment #3 from janus at gcc dot gnu.org 2012-11-28 09:40:33 UTC ---
The backtrace one gets on trunk is:

0x669272 gfc_conv_structure(gfc_se*, gfc_expr*, int)
        /home/jweil/gcc48/trunk/gcc/fortran/trans-expr.c:5971
0x667dbb gfc_conv_initializer(gfc_expr*, gfc_typespec*, tree_node*, bool, bool,
bool)
        /home/jweil/gcc48/trunk/gcc/fortran/trans-expr.c:5550
0x648799 gfc_get_symbol_decl(gfc_symbol*)
        /home/jweil/gcc48/trunk/gcc/fortran/trans-decl.c:1480


That line (trans-expr.c:5971) is the following assert:

  gcc_assert (expr->expr_type == EXPR_STRUCTURE);

which fails because we have an EXPR_FUNCTION.

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

--- Comment #4 from janus at gcc dot gnu.org 2012-11-28 10:10:40 UTC ---
For a case like this:

module test
  integer :: MPI_INTEGER = merge(4, 8, .false.)
end module 

we do not get an EXPR_FUNCTION in gfc_conv_initializer, but it is simplified to
EXPR_CONSTANT before. It seems that this does not work for derived types.

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

janus at gcc dot gnu.org changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
         AssignedTo|unassigned at gcc dot       |janus at gcc dot gnu.org
                   |gnu.org                     |

--- Comment #5 from janus at gcc dot gnu.org 2012-11-28 10:35:54 UTC ---
The following patch fixes it:

Index: gcc/fortran/simplify.c
===================================================================
--- gcc/fortran/simplify.c    (revision 193810)
+++ gcc/fortran/simplify.c    (working copy)
@@ -3973,8 +3973,10 @@ gfc_simplify_maskl (gfc_expr *i, gfc_expr *kind_ar
 gfc_expr *
 gfc_simplify_merge (gfc_expr *tsource, gfc_expr *fsource, gfc_expr *mask)
 {
-  if (tsource->expr_type != EXPR_CONSTANT
-      || fsource->expr_type != EXPR_CONSTANT
+  if ((tsource->expr_type != EXPR_CONSTANT
+       && tsource->expr_type != EXPR_STRUCTURE)
+      || (fsource->expr_type != EXPR_CONSTANT
+      && fsource->expr_type != EXPR_STRUCTURE)
       || mask->expr_type != EXPR_CONSTANT)
     return NULL;

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[burnus at gcc dot gnu.org]

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

--- Comment #6 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-11-28 10:48:58 UTC ---
(In reply to comment #2)
>   type(MPI_Datatype) :: MPI_INTEGER = merge(MPI_Datatype(4), MPI_Datatype(8),
> .false.)

The problem is related to having array PARAMETERs. For normal parameters,
simply their value is stored in the .mod file and always inserted when used.

For array parameters, a static array in read-only memory is created, which can
then be accessed at run time. That avoids replicating the information several
times. In addition, the expression is also stored in the .mod file.

Especially if constructors are involved, the current compile-time
simplification doesn't work that well. Additionally, the question is also
whether it always makes sense to expand constructors if one wants to simplify
code.

In any case, there is room for improvement. See also PR 44856 and PR 51260.

* * *

In gfc_simplify_merge, the compiler gives up when the type is not an
EXPR_CONSTANT:

3976      if (tsource->expr_type != EXPR_CONSTANT
3977          || fsource->expr_type != EXPR_CONSTANT
3978          || mask->expr_type != EXPR_CONSTANT)
3979        return NULL;


For the test case of this PR, one has an EXPR_STRUCTURE. Maybe replacing the
check by calls to gfc_is_constant_expr() is sufficient.

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

--- Comment #7 from janus at gcc dot gnu.org 2012-11-28 10:50:28 UTC ---
I think the following variant makes even more sense:

Index: gcc/fortran/simplify.c
===================================================================
--- gcc/fortran/simplify.c    (revision 193810)
+++ gcc/fortran/simplify.c    (working copy)
@@ -3973,9 +3973,7 @@ gfc_simplify_maskl (gfc_expr *i, gfc_expr *kind_ar
 gfc_expr *
 gfc_simplify_merge (gfc_expr *tsource, gfc_expr *fsource, gfc_expr *mask)
 {
-  if (tsource->expr_type != EXPR_CONSTANT
-      || fsource->expr_type != EXPR_CONSTANT
-      || mask->expr_type != EXPR_CONSTANT)
+  if (mask->expr_type != EXPR_CONSTANT)
     return NULL;

   return gfc_copy_expr (mask->value.logical ? tsource : fsource);


In order to simplify a MERGE expression, we don't need to rely on the TSOURCE
and FSOURCE arguments being constant. It's sufficient that the MASK is.

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[burnus at gcc dot gnu.org]

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

--- Comment #8 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-11-28 10:54:56 UTC ---
(In reply to comment #5)
> +       && tsource->expr_type != EXPR_STRUCTURE)

That's not okay: If you have
  integer, allocatable :: a(:), b(:)
one has an EXPR_STRUCTURE for "[a,b]" but not a constant expression. One has to
do a deep check. Well, gfc_is_constant_expr() is supposed to do this.

(Or should gfc_check_init_expr be used? I always confuse the two. For F90/F95
it makes a difference, for F200x it doesn't, and gfortran's usage is a mess.)

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[burnus at gcc dot gnu.org]

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

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

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

--- Comment #9 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-11-28 11:03:34 UTC ---
(In reply to comment #7)
> -  if (tsource->expr_type != EXPR_CONSTANT
> -      || fsource->expr_type != EXPR_CONSTANT
> -      || mask->expr_type != EXPR_CONSTANT)
> +  if (mask->expr_type != EXPR_CONSTANT)
>      return NULL;

That makes sense: If mask is a constant scalar, tsource or fsource can be used.
That patch is pre-approved.


However, at some point one has also to simplify:
  MERGE([1,2],[11,22], [.true.,.false.])
and for that case, all arguments have to be gfc_is_constant_expr(). At least
Fortran 2008 requires that the processor can do such a simplification. (One
could also handle the special case that mask is an array of only .true. or only
.false.)

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

--- Comment #10 from janus at gcc dot gnu.org 2012-11-28 12:16:59 UTC ---
(In reply to comment #9)
> (In reply to comment #7)
> > -  if (tsource->expr_type != EXPR_CONSTANT
> > -      || fsource->expr_type != EXPR_CONSTANT
> > -      || mask->expr_type != EXPR_CONSTANT)
> > +  if (mask->expr_type != EXPR_CONSTANT)
> >      return NULL;
> 
> That makes sense: If mask is a constant scalar, tsource or fsource can be used.
> That patch is pre-approved.

Unfortunately, it triggers a couple of testsuite regressions:

FAIL: gfortran.dg/bound_2.f90  -O0  execution test
FAIL: gfortran.dg/bound_7.f90  -O0  execution test
FAIL: gfortran.dg/char_cast_1.f90  -O   scan-tree-dump-times original
"6\\]\\[1\\]" 2
FAIL: gfortran.dg/merge_char_3.f90  -O0  execution test


The last one is understandable: It is supposed to check for different char
lengths beings passed to MERGE at runtime, but the call to MERGE is simplified
away (which is good!).

The third one is a tree-dump failure, where apparently the dump is different
because MERGE is simplified away now.

The first two are runtime checks, which are basically identical. Here is a
reduced test case for these:

  implicit none
  integer :: i(-1:1) = 0

  print *, lbound(merge(i,i,.true.))
  print *, ubound(merge(i,i,.true.))

end

Without the patch, this prints:
           1
           3
And with the patch:
          -1
           1

The output with the patch does look more reasonable to me. Or is there any
reason why the standard would demand the MERGE expression to have bounds of 1:3
instead of -1:1 ?

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[janus at gcc dot gnu.org]

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

--- Comment #11 from janus at gcc dot gnu.org 2012-11-28 12:22:02 UTC ---
(In reply to comment #10)
> The first two are runtime checks, which are basically identical. Here is a
> reduced test case for these:
> 
>   implicit none
>   integer :: i(-1:1) = 0
> 
>   print *, lbound(merge(i,i,.true.))
>   print *, ubound(merge(i,i,.true.))
> 
> end
> 
> Without the patch, this prints:
>            1
>            3
> And with the patch:
>           -1
>            1
> 
> The output with the patch does look more reasonable to me. Or is there any
> reason why the standard would demand the MERGE expression to have bounds of 1:3
> instead of -1:1 ?

At least all of ifort, sunf95 and g95 agree with the first variant (1:3).

[Btw, the bound_{2,7} test cases come from PR 29391.]

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[burnus at gcc dot gnu.org]

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

--- Comment #12 from Tobias Burnus <burnus at gcc dot gnu.org> 2012-11-28 14:54:33 UTC ---
(In reply to comment #10)
>   integer :: i(-1:1) = 0
>   print *, lbound(merge(i,i,.true.))

> Without the patch, this prints:
>            1
> And with the patch:
>           -1

Makes perfectly sense: For "lbound(array,dim)": "If ARRAY is a whole array and
either ARRAY is an assumed-size array of rank DIM or dimension DIM of ARRAY has
nonzero extent, LBOUND (ARRAY, DIM) has a value equal to the lower bound for
subscript DIM of ARRAY. Otherwise the result value is 1."
With "whole array  array component or array name without further quali\fcation
(6.5.2)"

Thus "lbound(i)" is a whole-array, "lbound(i(:))" or "lbound(merge(i,i,.true))"
is not.

I think the simplest it to replace "lbound (merge(i,i,.true.)" by "lbound( (i)
)" [e->expr_type = EXPR_OP && e->value.op.op == INTRINSIC_PARENTHESES].
Possibly only if expr_type == EXPR_VARIABLE as otherwise the
INTRINSIC_PARENTHESES will hamper further optimization (unless
-fno-protect-parens).

[Bug fortran/55501] [F03] ICE using MERGE in constant expr

[longb at cray dot com]

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

--- Comment #14 from Bill Long <longb at cray dot com> ---
Just a note that I'm now using

$ gf --version
GNU Fortran (MacPorts gcc49 4.9-20130609_0) 4.9.0 20130609 (experimental)
Copyright (C) 2013 Free Software Foundation, Inc.

and the original test case works with this version.