diff mbox

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

Message ID 4CD82C8F.9050408@frontier.com
State New
Headers show

Commit Message

Jerry DeLisle Nov. 8, 2010, 4:59 p.m. UTC
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.  ;)

Comments

Mikael Morin Nov. 8, 2010, 5:52 p.m. UTC | #1
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
diff mbox

Patch

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;