[PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Jakub Jelinek]

Hi!

On the gfortran.dg/pr103691.f90 testcase the Fortran ICE emits
  static real(kind=4) a[0] = {[0 ... -1]=2.0e+0};
That is an invalid RANGE_EXPR where the maximum is smaller than the minimum.

The following patch fixes that.  If TYPE_MAX_VALUE is smaller than
TYPE_MIN_VALUE, the array is empty and so doesn't need any initializer,
if the two are equal, we don't need to bother with a RANGE_EXPR and
can just use that INTEGER_CST as the index and finally for the 2+ values
in the range it uses a RANGE_EXPR as before.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2022-03-25  Jakub Jelinek  <jakub@redhat.com>

	PR fortran/103691
	* trans-array.cc (gfc_conv_array_initializer): If TYPE_MAX_VALUE is
	smaller than TYPE_MIN_VALUE (i.e. empty array), throw the initializer
	on the floor, if TYPE_MIN_VALUE is equal to TYPE_MAX_VALUE, use just
	the TYPE_MIN_VALUE as index instead of RANGE_EXPR.

--- gcc/fortran/trans-array.cc.jj	2022-02-04 14:36:55.113603791 +0100
+++ gcc/fortran/trans-array.cc	2022-03-24 16:14:58.334498775 +0100
@@ -6267,10 +6267,17 @@ gfc_conv_array_initializer (tree type, g
       else
 	gfc_conv_structure (&se, expr, 1);
 
-      CONSTRUCTOR_APPEND_ELT (v, build2 (RANGE_EXPR, gfc_array_index_type,
-					 TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
-					 TYPE_MAX_VALUE (TYPE_DOMAIN (type))),
-			      se.expr);
+      if (tree_int_cst_lt (TYPE_MAX_VALUE (TYPE_DOMAIN (type)),
+			   TYPE_MIN_VALUE (TYPE_DOMAIN (type))))
+	break;
+      else if (tree_int_cst_equal (TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
+				   TYPE_MAX_VALUE (TYPE_DOMAIN (type))))
+	range = TYPE_MIN_VALUE (TYPE_DOMAIN (type));
+      else
+	range = build2 (RANGE_EXPR, gfc_array_index_type,
+			TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
+			TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
+      CONSTRUCTOR_APPEND_ELT (v, range, se.expr);
       break;
 
     case EXPR_ARRAY:

	Jakub

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Tobias Burnus]

On 25.03.22 09:57, Jakub Jelinek via Fortran wrote:
> On the gfortran.dg/pr103691.f90 testcase the Fortran ICE emits
>    static real(kind=4) a[0] = {[0 ... -1]=2.0e+0};
> That is an invalid RANGE_EXPR where the maximum is smaller than the minimum.
>
> The following patch fixes that.  If TYPE_MAX_VALUE is smaller than
> TYPE_MIN_VALUE, the array is empty and so doesn't need any initializer,
> if the two are equal, we don't need to bother with a RANGE_EXPR and
> can just use that INTEGER_CST as the index and finally for the 2+ values
> in the range it uses a RANGE_EXPR as before.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

LGTM – thanks for taking care of Fortran patches and regressions.

> 2022-03-25  Jakub Jelinek  <jakub@redhat.com>
>
>       PR fortran/103691
>       * trans-array.cc (gfc_conv_array_initializer): If TYPE_MAX_VALUE is
>       smaller than TYPE_MIN_VALUE (i.e. empty array), throw the initializer
>       on the floor, if TYPE_MIN_VALUE is equal to TYPE_MAX_VALUE, use just
>       the TYPE_MIN_VALUE as index instead of RANGE_EXPR.

I am not sure whether "throw the initializer on the floor" is the best wording
for a changelog. I think I prefer a wording like "ignore the initializer" or
another less idiomatic expression. And I think a ';' before the second 'if'
also increases readability.

Tobias

> --- gcc/fortran/trans-array.cc.jj     2022-02-04 14:36:55.113603791 +0100
> +++ gcc/fortran/trans-array.cc        2022-03-24 16:14:58.334498775 +0100
> @@ -6267,10 +6267,17 @@ gfc_conv_array_initializer (tree type, g
>         else
>       gfc_conv_structure (&se, expr, 1);
>
> -      CONSTRUCTOR_APPEND_ELT (v, build2 (RANGE_EXPR, gfc_array_index_type,
> -                                      TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
> -                                      TYPE_MAX_VALUE (TYPE_DOMAIN (type))),
> -                           se.expr);
> +      if (tree_int_cst_lt (TYPE_MAX_VALUE (TYPE_DOMAIN (type)),
> +                        TYPE_MIN_VALUE (TYPE_DOMAIN (type))))
> +     break;
> +      else if (tree_int_cst_equal (TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
> +                                TYPE_MAX_VALUE (TYPE_DOMAIN (type))))
> +     range = TYPE_MIN_VALUE (TYPE_DOMAIN (type));
> +      else
> +     range = build2 (RANGE_EXPR, gfc_array_index_type,
> +                     TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
> +                     TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
> +      CONSTRUCTOR_APPEND_ELT (v, range, se.expr);
>         break;
>
>       case EXPR_ARRAY:
>
>       Jakub
>
-----------------
Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Richard Biener]

On Fri, Mar 25, 2022 at 11:13 AM Tobias Burnus <tobias@codesourcery.com> wrote:
>
> On 25.03.22 09:57, Jakub Jelinek via Fortran wrote:
> > On the gfortran.dg/pr103691.f90 testcase the Fortran ICE emits
> >    static real(kind=4) a[0] = {[0 ... -1]=2.0e+0};
> > That is an invalid RANGE_EXPR where the maximum is smaller than the minimum.
> >
> > The following patch fixes that.  If TYPE_MAX_VALUE is smaller than
> > TYPE_MIN_VALUE, the array is empty and so doesn't need any initializer,
> > if the two are equal, we don't need to bother with a RANGE_EXPR and
> > can just use that INTEGER_CST as the index and finally for the 2+ values
> > in the range it uses a RANGE_EXPR as before.
> >
> > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> LGTM – thanks for taking care of Fortran patches and regressions.
>
> > 2022-03-25  Jakub Jelinek  <jakub@redhat.com>
> >
> >       PR fortran/103691
> >       * trans-array.cc (gfc_conv_array_initializer): If TYPE_MAX_VALUE is
> >       smaller than TYPE_MIN_VALUE (i.e. empty array), throw the initializer
> >       on the floor, if TYPE_MIN_VALUE is equal to TYPE_MAX_VALUE, use just
> >       the TYPE_MIN_VALUE as index instead of RANGE_EXPR.
>
> I am not sure whether "throw the initializer on the floor" is the best wording
> for a changelog. I think I prefer a wording like "ignore the initializer" or
> another less idiomatic expression. And I think a ';' before the second 'if'
> also increases readability.

Can there be side-effects in those initializer elements in Fortran?

Richard.

> Tobias
>
> > --- gcc/fortran/trans-array.cc.jj     2022-02-04 14:36:55.113603791 +0100
> > +++ gcc/fortran/trans-array.cc        2022-03-24 16:14:58.334498775 +0100
> > @@ -6267,10 +6267,17 @@ gfc_conv_array_initializer (tree type, g
> >         else
> >       gfc_conv_structure (&se, expr, 1);
> >
> > -      CONSTRUCTOR_APPEND_ELT (v, build2 (RANGE_EXPR, gfc_array_index_type,
> > -                                      TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
> > -                                      TYPE_MAX_VALUE (TYPE_DOMAIN (type))),
> > -                           se.expr);
> > +      if (tree_int_cst_lt (TYPE_MAX_VALUE (TYPE_DOMAIN (type)),
> > +                        TYPE_MIN_VALUE (TYPE_DOMAIN (type))))
> > +     break;
> > +      else if (tree_int_cst_equal (TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
> > +                                TYPE_MAX_VALUE (TYPE_DOMAIN (type))))
> > +     range = TYPE_MIN_VALUE (TYPE_DOMAIN (type));
> > +      else
> > +     range = build2 (RANGE_EXPR, gfc_array_index_type,
> > +                     TYPE_MIN_VALUE (TYPE_DOMAIN (type)),
> > +                     TYPE_MAX_VALUE (TYPE_DOMAIN (type)));
> > +      CONSTRUCTOR_APPEND_ELT (v, range, se.expr);
> >         break;
> >
> >       case EXPR_ARRAY:
> >
> >       Jakub
> >
> -----------------
> Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Jakub Jelinek]

On Fri, Mar 25, 2022 at 12:16:40PM +0100, Richard Biener wrote:
> On Fri, Mar 25, 2022 at 11:13 AM Tobias Burnus <tobias@codesourcery.com> wrote:
> >
> > On 25.03.22 09:57, Jakub Jelinek via Fortran wrote:
> > > On the gfortran.dg/pr103691.f90 testcase the Fortran ICE emits
> > >    static real(kind=4) a[0] = {[0 ... -1]=2.0e+0};
> > > That is an invalid RANGE_EXPR where the maximum is smaller than the minimum.
> > >
> > > The following patch fixes that.  If TYPE_MAX_VALUE is smaller than
> > > TYPE_MIN_VALUE, the array is empty and so doesn't need any initializer,
> > > if the two are equal, we don't need to bother with a RANGE_EXPR and
> > > can just use that INTEGER_CST as the index and finally for the 2+ values
> > > in the range it uses a RANGE_EXPR as before.
> > >
> > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> >
> > LGTM – thanks for taking care of Fortran patches and regressions.
> >
> > > 2022-03-25  Jakub Jelinek  <jakub@redhat.com>
> > >
> > >       PR fortran/103691
> > >       * trans-array.cc (gfc_conv_array_initializer): If TYPE_MAX_VALUE is
> > >       smaller than TYPE_MIN_VALUE (i.e. empty array), throw the initializer
> > >       on the floor, if TYPE_MIN_VALUE is equal to TYPE_MAX_VALUE, use just
> > >       the TYPE_MIN_VALUE as index instead of RANGE_EXPR.
> >
> > I am not sure whether "throw the initializer on the floor" is the best wording
> > for a changelog. I think I prefer a wording like "ignore the initializer" or
> > another less idiomatic expression. And I think a ';' before the second 'if'
> > also increases readability.
> 
> Can there be side-effects in those initializer elements in Fortran?

For PARAMETERs certainly not, those need to be constant.
Even otherwise, this is in a routine that does
  /* Create a constructor from the list of elements.  */
  tmp = build_constructor (type, v);
  TREE_CONSTANT (tmp) = 1;
  return tmp;
at the end so I wouldn't expect side-effects anywhere.

Also, I think typically in the Fortran FE side-effects would go into
se.pre and se.post sequences, not into se.expr, and this routine
doesn't emit those se.pre/se.post sequences anywhere, so presumably it
assumes they don't exist.

What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i; },
is that applying the side-effects 11 times or once ?


	Jakub

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Richard Biener]

On Fri, Mar 25, 2022 at 12:34 PM Jakub Jelinek <jakub@redhat.com> wrote:
>
> On Fri, Mar 25, 2022 at 12:16:40PM +0100, Richard Biener wrote:
> > On Fri, Mar 25, 2022 at 11:13 AM Tobias Burnus <tobias@codesourcery.com> wrote:
> > >
> > > On 25.03.22 09:57, Jakub Jelinek via Fortran wrote:
> > > > On the gfortran.dg/pr103691.f90 testcase the Fortran ICE emits
> > > >    static real(kind=4) a[0] = {[0 ... -1]=2.0e+0};
> > > > That is an invalid RANGE_EXPR where the maximum is smaller than the minimum.
> > > >
> > > > The following patch fixes that.  If TYPE_MAX_VALUE is smaller than
> > > > TYPE_MIN_VALUE, the array is empty and so doesn't need any initializer,
> > > > if the two are equal, we don't need to bother with a RANGE_EXPR and
> > > > can just use that INTEGER_CST as the index and finally for the 2+ values
> > > > in the range it uses a RANGE_EXPR as before.
> > > >
> > > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
> > >
> > > LGTM – thanks for taking care of Fortran patches and regressions.
> > >
> > > > 2022-03-25  Jakub Jelinek  <jakub@redhat.com>
> > > >
> > > >       PR fortran/103691
> > > >       * trans-array.cc (gfc_conv_array_initializer): If TYPE_MAX_VALUE is
> > > >       smaller than TYPE_MIN_VALUE (i.e. empty array), throw the initializer
> > > >       on the floor, if TYPE_MIN_VALUE is equal to TYPE_MAX_VALUE, use just
> > > >       the TYPE_MIN_VALUE as index instead of RANGE_EXPR.
> > >
> > > I am not sure whether "throw the initializer on the floor" is the best wording
> > > for a changelog. I think I prefer a wording like "ignore the initializer" or
> > > another less idiomatic expression. And I think a ';' before the second 'if'
> > > also increases readability.
> >
> > Can there be side-effects in those initializer elements in Fortran?
>
> For PARAMETERs certainly not, those need to be constant.
> Even otherwise, this is in a routine that does
>   /* Create a constructor from the list of elements.  */
>   tmp = build_constructor (type, v);
>   TREE_CONSTANT (tmp) = 1;
>   return tmp;
> at the end so I wouldn't expect side-effects anywhere.

Ah, didn't see that.

> Also, I think typically in the Fortran FE side-effects would go into
> se.pre and se.post sequences, not into se.expr, and this routine
> doesn't emit those se.pre/se.post sequences anywhere, so presumably it
> assumes they don't exist.
>
> What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i; },
> is that applying the side-effects 11 times or once ?

11 times is what is documented.

Richard.

>
>
>         Jakub
>

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Jakub Jelinek]

On Fri, Mar 25, 2022 at 01:13:06PM +0100, Richard Biener wrote:
> > Also, I think typically in the Fortran FE side-effects would go into
> > se.pre and se.post sequences, not into se.expr, and this routine
> > doesn't emit those se.pre/se.post sequences anywhere, so presumably it
> > assumes they don't exist.
> >
> > What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i; },
> > is that applying the side-effects 11 times or once ?
> 
> 11 times is what is documented.

Then [0..-1] = ++i should be 0 times the side-effect.

	Jakub

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Thomas Koenig]

On 25.03.22 12:34, Jakub Jelinek via Fortran wrote:
> What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i;
> }, is that applying the side-effects 11 times or once ?

For side effects during the evaluation of expression, Fortran has a
clear "if you depend on it, it's your fault" rule.  In F 2018, it says

10.1.7  Evaluation of operands

1 It is not necessary for a processor to evaluate all of the operands of
an expression, or to evaluate entirely each operand, if the value of the
expression can be determined otherwise.

Also, the semantics of

a(a:b) = expr

say that the expression on the LHS is evaluated only once before
assignment.  So, anything that looks like that should be translated
to

tmp = ++i;
[0..10] = tmp;

Re: [PATCH] fortran: Fix up initializers of param(0) PARAMETERs [PR103691]

[Richard Biener]

> Am 26.03.2022 um 12:28 schrieb Thomas Koenig <tkoenig@netcologne.de>:
> 
> On 25.03.22 12:34, Jakub Jelinek via Fortran wrote:
>> What is the behavior with a RANGE_EXPR when one has { [0..10] = ++i;
>> }, is that applying the side-effects 11 times or once ?
> 
> For side effects during the evaluation of expression, Fortran has a
> clear "if you depend on it, it's your fault" rule.  In F 2018, it says
> 
> 10.1.7  Evaluation of operands
> 
> 1 It is not necessary for a processor to evaluate all of the operands of
> an expression, or to evaluate entirely each operand, if the value of the
> expression can be determined otherwise.
> 
> Also, the semantics of
> 
> a(a:b) = expr
> 
> say that the expression on the LHS is evaluated only once before
> assignment.  So, anything that looks like that should be translated
> to
> 
> tmp = ++i;
> [0..10] = tmp;

Note I was not trying to question middle-end semantic here but gfortran se_expr (?) one. Is there a Fortan input where Jakob’s patch would cause a side-effect to be dropped and is that valid?

Richard.

> 
>