Re: [fortran patch] Don't use TREE_LISTs for storing arguments to intrinsic functions

["Lee Millward" <lee dot millward at codesourcery dot com>]

Hi FX

On 5/8/07, FX Coudert <fxcoudert@gmail.com> wrote:
> > This patch modifies the handing of intrinsic functions to not use a
> > TREE_LIST for storing the arguments to instead use a stack allocated
> > array and use the new CALL_EXPR constructors.
>
> It might be a stupid question, but what is the benefit of this new
> approach? It doesn't look to me like TREE_LIST are very expensive to
> build, and we're duplicating information a bit by writing the number
> of arguments in multiple places (in the array declaration, as
> argument to gfc_conv_intrinsic_function_args, as argument to
> build_call_expr).

As Brooks has already mentioned this is part of the CALL_EXPR cleanup
work. IIRC the Fortran front-end was the last big offender in using
the old build_function_call_expr instead of the new constructors added
during the representation changes.

> > For most of the callers, the number of arguments required is already
> > known. However, in some cases this isn't true so a new function
> > (gfc_intrinsic_argument_list_length) has been added that returns the
> > number of arguments for an intrinsic function to use for allocating
> > the arguments array.
>
>
> One thing I don't understand... they might be due to bad copy-pasting
> on your side, or the late hour in my timezone:
>
> > @@ -347,7 +370,7 @@ gfc_conv_intrinsic_aint (gfc_se * se, gf
> >  {
> >    tree type;
> >    tree itype;
> > -  tree arg;
> > +  tree args[2];
> >    tree tmp;
> >    tree cond;
> >    mpfr_t huge;
> > @@ -402,21 +425,20 @@ gfc_conv_intrinsic_aint (gfc_se * se, gf
> >
> >    /* Evaluate the argument.  */
> >    gcc_assert (expr->value.function.actual->expr);
> > -  arg = gfc_conv_intrinsic_function_args (se, expr);
> > +  gfc_conv_intrinsic_function_args (se, expr, args, 2);
>
> Why are there two args in gfc_conv_intrinsic_aint? Especially when
> only one is used later? The same question applies to
> gfc_conv_intrinsic_int and gfc_conv_intrinsic_conversion.

I think this is a leftover of an earlier version of this patch, I'll
fix it for the revised version.

> > @@ -481,10 +502,9 @@ gfc_conv_intrinsic_int (gfc_se * se, gfc
> >  static void
> >  gfc_conv_intrinsic_imagpart (gfc_se * se, gfc_expr * expr)
> >  {
> > -  tree arg;
> > +  tree arg = NULL_TREE;
>
> Why is that arg value initialized, while the others above (the "tree
> args[2];") are not? Same thing for gfc_conv_intrinsic_imagpart.

GCC warns when you take the address of an unitialised variable so it
needs to be initialised to suppress the warning, even though this is
essentially useless.

> > +  fndecl = build_addr (fndecl, current_function_decl);
> > +  se->expr = build_call_array (rettype, fndecl, num_args, args);
> > +  se->expr = se->expr;
>
> Can you explain shortly why we now need this double manipulation with
> build_add/build_call_array? (especially build_addr, and why it uses
> current_function_decl)

Without wrapping the function declaration within an ADDR_EXPR we trip
over an assert in the middle-end. I can't remember the exact location
off hand but this seems to be in line with the way the old code used
to work.

> Minor issues:
>
> > +static int
> > +gfc_intrinsic_argument_list_length (gfc_expr * expr)
> > +{
> > +  int n = 0;
> > +  gfc_actual_arglist *actual;
> > +  gfc_intrinsic_arg  *formal = expr->value.function.isym->formal;;
>
> Mind the ;; at the end of this last line.
>
> > +  num_args = gfc_intrinsic_argument_list_length (expr);
> > +  args = alloca (sizeof (tree) * num_args);
> > +
> > +  gfc_conv_intrinsic_function_args (se, expr, args, num_args);;
>
> Same thing here.

I'll fix those in the revised version.

Cheers,
Lee