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

login
register
mail settings
Submitter Jerry DeLisle
Date Nov. 9, 2010, 11:14 p.m.
Message ID <4CD9D5CC.2040905@frontier.com>
Download mbox | patch
Permalink /patch/70585/
State New
Headers show

Comments

Jerry DeLisle - Nov. 9, 2010, 11:14 p.m.
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.
Mikael Morin - Nov. 9, 2010, 11:37 p.m.
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

Patch

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;