[patch, fortran] PR46331 Compilation time long with simple function in array constructor

[patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Jerry DeLisle]

[-- Attachment #1: Type: text/plain, Size: 710 bytes --]

Hi all,

The problem here is that we are expanding a large array constructor that 
contains a function that does not reduce (ie. rand(0)). The solution is a small 
adjustment to gfc_is_constant_expr which is used to determine whether or not to 
expand a constructor.  In this case, simply treating an non pure function as not 
constant.

For the test case, without the patch, compile time is approximately 19 seconds. 
With the patch, compilation is about 0.1 seconds.

I don't think a new test case is needed.

Ok for trunk?

Regards,

Jerry

2010-11-07  Jerry DeLisle  <jvdelisle@gcc.gnu.org>

	PR fortran/46331
	* expr.c (gfc_is_constant_expr): Treat intrinsic functions that are not
	pure as not constant.

[-- Attachment #2: pr46331.diff --]
[-- Type: text/x-patch, Size: 554 bytes --]

Index: expr.c
===================================================================
--- expr.c	(revision 166382)
+++ expr.c	(working copy)
@@ -923,7 +923,8 @@ gfc_is_constant_expr (gfc_expr *e)
 	return 1;
 
       /* Call to intrinsic with at least one argument.  */
-      if (e->value.function.isym && e->value.function.actual)
+      if (e->symtree && e->symtree->n.sym->attr.pure
+	  && e->value.function.isym && e->value.function.actual)
 	{
 	  for (arg = e->value.function.actual; arg; arg = arg->next)
 	    if (!gfc_is_constant_expr (arg->expr))

Re: [patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Mikael Morin]

On Monday 08 November 2010 03:40:32 Jerry DeLisle wrote:
> Hi all,
> 
> The problem here is that we are expanding a large array constructor that
> contains a function that does not reduce (ie. rand(0)). The solution is a
> small adjustment to gfc_is_constant_expr which is used to determine
> whether or not to expand a constructor.  In this case, simply treating an
> non pure function as not constant.
> 
Hello,

I think it may miss some simplification opportunities. 
For example, transformational functions and inquiry functions are not marked 
pure as far as I know. But some of them have simplification functions. 
As the klass is not saved anywhere in intrinsic.c's add_sym (at least not in 
the CLASS_IMPURE case), maybe would it work to check the presence of a 
simplification function pointer ? Or maybe gfc_intrinsic_sym's flags !pure && 
!inquiry && !transformational would match CLASS_IMPURE ?

Mikael

PS: Is it expected that the and/or/xor procedures are marked as CLASS_IMPURE ?

Re: [patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Tobias Burnus]

On 11/08/2010 01:13 PM, Mikael Morin wrote:
> PS: Is it expected that the and/or/xor procedures are marked as CLASS_IMPURE

The CLASS_* names come from the Fortran standard (where the term 
"impure" is a F2008 addition). Well, looking at the standard you find:

13.7.10 ALL (MASK [, DIM])
Class. Transformational function.

The procedures "OR" and "XOR" are GNU extensions; the manual does not 
tell which kind of functions they are, but the standard Fortran "IOR" 
and "IEOR", which are the Fortran-standard replacements, are "elemental 
functions". If one changes OR and XOR to elemental, one also needs to 
update the documentation, cf. 
http://gcc.gnu.org/onlinedocs/gfortran/OR.html and 
http://gcc.gnu.org/onlinedocs/gfortran/XOR.html

Tobias

Re: [patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Jerry DeLisle]

[-- Attachment #1: Type: text/plain, Size: 1668 bytes --]

On 11/08/2010 04:13 AM, Mikael Morin wrote:
> On Monday 08 November 2010 03:40:32 Jerry DeLisle wrote:
>> Hi all,
>>
>> The problem here is that we are expanding a large array constructor that
>> contains a function that does not reduce (ie. rand(0)). The solution is a
>> small adjustment to gfc_is_constant_expr which is used to determine
>> whether or not to expand a constructor.  In this case, simply treating an
>> non pure function as not constant.
>>
> Hello,
>
> I think it may miss some simplification opportunities.
> For example, transformational functions and inquiry functions are not marked
> pure as far as I know. But some of them have simplification functions.
> As the klass is not saved anywhere in intrinsic.c's add_sym (at least not in
> the CLASS_IMPURE case), maybe would it work to check the presence of a
> simplification function pointer ? Or maybe gfc_intrinsic_sym's flags !pure&&
> !inquiry&&  !transformational would match CLASS_IMPURE ?

All of the checks for these things are in check_specification_function which is 
just above gfc_is_constant_expr in expr.c.  The check_specification_function 
refers to section 7.1.7 for suitable initialization expressions. My patch is not 
attempting to change any of this.  If there is a problem there it is a new PR, ;)

However, the attached new patch also fixes the subject PR by rearranging the 
order of checks in gfc_is_constant_expr, allowing check_specification_function 
to do its job.  This also regression tests fine.

OK for trunk?

Jerry


>
> Mikael
>
> PS: Is it expected that the and/or/xor procedures are marked as CLASS_IMPURE ?
>

PS: I hope Tobias answered this for you.  ;)


[-- Attachment #2: pr46331-b.diff --]
[-- Type: text/x-patch, Size: 893 bytes --]

Index: expr.c
===================================================================
--- expr.c	(revision 166426)
+++ expr.c	(working copy)
@@ -918,22 +918,20 @@ gfc_is_constant_expr (gfc_expr *e)
     case EXPR_FUNCTION:
     case EXPR_PPC:
     case EXPR_COMPCALL:
-      /* Specification functions are constant.  */
-      if (check_specification_function (e) == MATCH_YES)
-	return 1;
-
       /* Call to intrinsic with at least one argument.  */
       if (e->value.function.isym && e->value.function.actual)
 	{
 	  for (arg = e->value.function.actual; arg; arg = arg->next)
 	    if (!gfc_is_constant_expr (arg->expr))
 	      return 0;
-
-	  return 1;
 	}
-      else
-	return 0;
 
+      /* Specification functions are constant.  */
+      if (check_specification_function (e) == MATCH_YES)
+	return 1;
+
+      return 0;
+
     case EXPR_CONSTANT:
     case EXPR_NULL:
       return 1;

Re: [patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Mikael Morin]

On Monday 08 November 2010 17:59:59 Jerry DeLisle wrote:
> On 11/08/2010 04:13 AM, Mikael Morin wrote:
> > On Monday 08 November 2010 03:40:32 Jerry DeLisle wrote:
> >> Hi all,
> >> 
> >> The problem here is that we are expanding a large array constructor that
> >> contains a function that does not reduce (ie. rand(0)). The solution is
> >> a small adjustment to gfc_is_constant_expr which is used to determine
> >> whether or not to expand a constructor.  In this case, simply treating
> >> an non pure function as not constant.
> > 
> > Hello,
> > 
> > I think it may miss some simplification opportunities.
> > For example, transformational functions and inquiry functions are not
> > marked pure as far as I know. But some of them have simplification
> > functions. As the klass is not saved anywhere in intrinsic.c's add_sym
> > (at least not in the CLASS_IMPURE case), maybe would it work to check
> > the presence of a simplification function pointer ? Or maybe
> > gfc_intrinsic_sym's flags !pure&& !inquiry&&  !transformational would
> > match CLASS_IMPURE ?
> 
> All of the checks for these things are in check_specification_function
> which is just above gfc_is_constant_expr in expr.c.  The
> check_specification_function refers to section 7.1.7 for suitable
> initialization expressions. My patch is not attempting to change any of
> this.  If there is a problem there it is a new PR, ;)
Sorry for missing check_specification_function. 

> 
> However, the attached new patch also fixes the subject PR by rearranging
> the order of checks in gfc_is_constant_expr, allowing
> check_specification_function to do its job.  This also regression tests
> fine.
Well, I like this patch, but I think it is actually worse. 
In check_specification_functions, there is the !sym->intrinsic condition, so 
it will miss constant expressions for any intrinsic. I mean, not only any, all 
intrinsics. Grrrr!! Not only all, I mean for every intrinsic. All of them. 
* intrinsics.

> 
> OK for trunk?
> 
> Jerry
> 
> > Mikael
> > 
> > PS: Is it expected that the and/or/xor procedures are marked as
> > CLASS_IMPURE ?
> 
> PS: I hope Tobias answered this for you.  ;)
Yes, he did with a lengthy explanation telling it was a gnu legacy feature 
which is essentially the same as saying "Nobody cares".
He also deterred anyone to propose a patch by including the word 
"documentation" in his answer. 

Mikael

Re: [patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Jerry DeLisle]

[-- Attachment #1: Type: text/plain, Size: 2648 bytes --]

On 11/08/2010 08:59 AM, Jerry DeLisle wrote:
> On 11/08/2010 04:13 AM, Mikael Morin wrote:
>> On Monday 08 November 2010 03:40:32 Jerry DeLisle wrote:
>>> Hi all,
>>>
>>> The problem here is that we are expanding a large array constructor that
>>> contains a function that does not reduce (ie. rand(0)). The solution is a
>>> small adjustment to gfc_is_constant_expr which is used to determine
>>> whether or not to expand a constructor. In this case, simply treating an
>>> non pure function as not constant.
>>>
>> Hello,
>>
>> I think it may miss some simplification opportunities.
>> For example, transformational functions and inquiry functions are not marked
>> pure as far as I know. But some of them have simplification functions.
>> As the klass is not saved anywhere in intrinsic.c's add_sym (at least not in
>> the CLASS_IMPURE case), maybe would it work to check the presence of a
>> simplification function pointer ? Or maybe gfc_intrinsic_sym's flags !pure&&
>> !inquiry&& !transformational would match CLASS_IMPURE ?
>
> All of the checks for these things are in check_specification_function which is
> just above gfc_is_constant_expr in expr.c. The check_specification_function
> refers to section 7.1.7 for suitable initialization expressions. My patch is not
> attempting to change any of this. If there is a problem there it is a new PR, ;)
>
> However, the attached new patch also fixes the subject PR by rearranging the
> order of checks in gfc_is_constant_expr, allowing check_specification_function
> to do its job. This also regression tests fine.
>

After some discussion on IRC I have arrived at the attached patch with Mikael's 
help. We found some inconsistency between setting of sym->attr.pure and setting 
of e->value.function.isym->pure.  Mikael came up with the suggested fix for that 
and this new patch passes regression testing and is easier to follow.  I got rid 
of the static check_specification_function and just in-lined that part in the 
only place that it was used.

OK for trunk?

Regards,

Jerry

PS Thanks Mikael and for those curious, the -fdump-tree-original files size 
drops from 1.4 Mbytes to under 4kbytes on the original test case.

2010-11-09  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
	    Mikael Morin   <mikael@gcc.gnu.org>

	PR fortran/46331
	* intrinsic.c: Correctly set the pure attributes for intrinsic
	functions.
	* expr.c (check_specification_function): Remove this function and move
	its code into gfc_is_constant_expr. (gfc_is_constant_expr): Change the
	order of checks by checking for non-constant arguments first.  Then,
	check for initialization functions, followed by intrinsics.

[-- Attachment #2: pr46331-c.diff --]
[-- Type: text/x-patch, Size: 3730 bytes --]

Index: intrinsic.c
===================================================================
--- intrinsic.c	(revision 166469)
+++ intrinsic.c	(working copy)
@@ -274,10 +274,7 @@ add_sym (const char *name, gfc_isym_id id, enum kl
       strcat (buf, name);
       next_sym->lib_name = gfc_get_string (buf);
 
-      /* There are no IMPURE ELEMENTAL intrinsics, thus the ELEMENTAL class
-	 also implies PURE.  Additionally, there's the PURE class itself.  */
-      next_sym->pure = (cl == CLASS_ELEMENTAL || cl == CLASS_PURE);
-
+      next_sym->pure = (cl != CLASS_IMPURE);
       next_sym->elemental = (cl == CLASS_ELEMENTAL);
       next_sym->inquiry = (cl == CLASS_INQUIRY);
       next_sym->transformational = (cl == CLASS_TRANSFORMATIONAL);
@@ -3370,8 +3367,6 @@ add_char_conversions (void)
 void
 gfc_intrinsic_init_1 (void)
 {
-  int i;
-
   nargs = nfunc = nsub = nconv = 0;
 
   /* Create a namespace to hold the resolved intrinsic symbols.  */
@@ -3404,15 +3399,6 @@ gfc_intrinsic_init_1 (void)
 
   /* Character conversion intrinsics need to be treated separately.  */
   add_char_conversions ();
-
-  /* Set the pure flag.  All intrinsic functions are pure, and
-     intrinsic subroutines are pure if they are elemental.  */
-
-  for (i = 0; i < nfunc; i++)
-    functions[i].pure = 1;
-
-  for (i = 0; i < nsub; i++)
-    subroutines[i].pure = subroutines[i].elemental;
 }
 
 
Index: expr.c
===================================================================
--- expr.c	(revision 166469)
+++ expr.c	(working copy)
@@ -868,31 +868,6 @@ done:
 }
 
 
-static match
-check_specification_function (gfc_expr *e)
-{
-  gfc_symbol *sym;
-
-  if (!e->symtree)
-    return MATCH_NO;
-
-  sym = e->symtree->n.sym;
-
-  /* F95, 7.1.6.2; F2003, 7.1.7  */
-  if (sym
-      && sym->attr.function
-      && sym->attr.pure
-      && !sym->attr.intrinsic
-      && !sym->attr.recursive
-      && sym->attr.proc != PROC_INTERNAL
-      && sym->attr.proc != PROC_ST_FUNCTION
-      && sym->attr.proc != PROC_UNKNOWN
-      && sym->formal == NULL)
-    return MATCH_YES;
-
-  return MATCH_NO;
-}
-
 /* Function to determine if an expression is constant or not.  This
    function expects that the expression has already been simplified.  */
 
@@ -901,6 +876,7 @@ gfc_is_constant_expr (gfc_expr *e)
 {
   gfc_constructor *c;
   gfc_actual_arglist *arg;
+  gfc_symbol *sym;
 
   if (e == NULL)
     return 1;
@@ -918,22 +894,41 @@ gfc_is_constant_expr (gfc_expr *e)
     case EXPR_FUNCTION:
     case EXPR_PPC:
     case EXPR_COMPCALL:
-      /* Specification functions are constant.  */
-      if (check_specification_function (e) == MATCH_YES)
-	return 1;
-
       /* Call to intrinsic with at least one argument.  */
       if (e->value.function.isym && e->value.function.actual)
 	{
 	  for (arg = e->value.function.actual; arg; arg = arg->next)
 	    if (!gfc_is_constant_expr (arg->expr))
 	      return 0;
-
-	  return 1;
 	}
-      else
-	return 0;
 
+      /* Make sure we have a symbol.  */
+      gcc_assert (e->symtree);
+
+      sym = e->symtree->n.sym;
+    
+      /* Specification functions are constant.  */
+      /* F95, 7.1.6.2; F2003, 7.1.7  */
+      if (sym
+	  && sym->attr.function
+	  && sym->attr.pure
+	  && !sym->attr.intrinsic
+	  && !sym->attr.recursive
+	  && sym->attr.proc != PROC_INTERNAL
+	  && sym->attr.proc != PROC_ST_FUNCTION
+	  && sym->attr.proc != PROC_UNKNOWN
+	  && sym->formal == NULL)
+	return 1;
+
+      if (e->value.function.isym
+	  && (e->value.function.isym->elemental
+	  || e->value.function.isym->pure
+	  || e->value.function.isym->inquiry
+	  || e->value.function.isym->transformational))
+	return 1;
+
+      return 0;
+
     case EXPR_CONSTANT:
     case EXPR_NULL:
       return 1;

Re: [patch, fortran] PR46331 Compilation time long with simple function in array constructor

[Mikael Morin]

On Wednesday 10 November 2010 00:14:20 Jerry DeLisle wrote:
> After some discussion on IRC I have arrived at the attached patch with
> Mikael's help. We found some inconsistency between setting of
> sym->attr.pure and setting of e->value.function.isym->pure.  Mikael came
> up with the suggested fix for that and this new patch passes regression
> testing and is easier to follow.  I got rid of the static
> check_specification_function and just in-lined that part in the only place
> that it was used.
> 
> OK for trunk?
> 
> Regards,
> 
> Jerry
> 
> PS Thanks Mikael and for those curious, the -fdump-tree-original files size
> drops from 1.4 Mbytes to under 4kbytes on the original test case.
> 
> 2010-11-09  Jerry DeLisle  <jvdelisle@gcc.gnu.org>
> 	    Mikael Morin   <mikael@gcc.gnu.org>
> 
> 	PR fortran/46331
> 	* intrinsic.c: Correctly set the pure attributes for intrinsic
> 	functions.
> 	* expr.c (check_specification_function): Remove this function and move
> 	its code into gfc_is_constant_expr. (gfc_is_constant_expr): Change the
> 	order of checks by checking for non-constant arguments first.  Then,
> 	check for initialization functions, followed by intrinsics.
> 
> 
> pr46331-c.diff
> Index: expr.c
> ===================================================================
> --- expr.c      (revision 166469)
> +++ expr.c      (working copy)
> @@ -868,31 +868,6 @@ done:
>  }
>  
>  
> -static match
> -check_specification_function (gfc_expr *e)
> -{
> -  gfc_symbol *sym;
> -
> -  if (!e->symtree)
> -    return MATCH_NO;
> -
> -  sym = e->symtree->n.sym;
> -
> -  /* F95, 7.1.6.2; F2003, 7.1.7  */
> -  if (sym
> -      && sym->attr.function
> -      && sym->attr.pure
> -      && !sym->attr.intrinsic
> -      && !sym->attr.recursive
> -      && sym->attr.proc != PROC_INTERNAL
> -      && sym->attr.proc != PROC_ST_FUNCTION
> -      && sym->attr.proc != PROC_UNKNOWN
> -      && sym->formal == NULL)
> -    return MATCH_YES;
> -
> -  return MATCH_NO;
> -}
> -
>  /* Function to determine if an expression is constant or not.  This
>     function expects that the expression has already been simplified.  */
>  
> @@ -901,6 +876,7 @@ gfc_is_constant_expr (gfc_expr *e)
>  {
>    gfc_constructor *c;
>    gfc_actual_arglist *arg;
> +  gfc_symbol *sym;
>  
>    if (e == NULL)
>      return 1;
> @@ -918,22 +894,41 @@ gfc_is_constant_expr (gfc_expr *e)
>      case EXPR_FUNCTION:
>      case EXPR_PPC:
>      case EXPR_COMPCALL:
> -      /* Specification functions are constant.  */
> -      if (check_specification_function (e) == MATCH_YES)
> -       return 1;
> -
>        /* Call to intrinsic with at least one argument.  */
>        if (e->value.function.isym && e->value.function.actual)
>         {
>           for (arg = e->value.function.actual; arg; arg = arg->next)
>             if (!gfc_is_constant_expr (arg->expr))
>               return 0;
> -
> -         return 1;
>         }
> -      else
> -       return 0;
>  
> +      /* Make sure we have a symbol.  */
> +      gcc_assert (e->symtree);
> +
> +      sym = e->symtree->n.sym;
> +    
> +      /* Specification functions are constant.  */
> +      /* F95, 7.1.6.2; F2003, 7.1.7  */
> +      if (sym
> +         && sym->attr.function
> +         && sym->attr.pure
> +         && !sym->attr.intrinsic
> +         && !sym->attr.recursive
> +         && sym->attr.proc != PROC_INTERNAL
> +         && sym->attr.proc != PROC_ST_FUNCTION
> +         && sym->attr.proc != PROC_UNKNOWN
> +         && sym->formal == NULL)
> +       return 1;
> +
> +      if (e->value.function.isym
> +         && (e->value.function.isym->elemental
> +         || e->value.function.isym->pure
> +         || e->value.function.isym->inquiry
> +         || e->value.function.isym->transformational))
> +       return 1;
As now the pure flag is set with cl != CLASS_IMPURE, checking the pure flag 
only should be sufficient. I don't really care, so Jerry, you can keep this 
version (which you have regression tested I suppose ?) if you prefer it. 
Please indent more the three || conditions.

OK with the above comment. Thanks for the patch (and for your patience)
Mikael