This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: [fortran patch] Don't use TREE_LISTs for storing arguments to intrinsic functions
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