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

[FX Coudert <fxcoudert at gmail dot com>]

Hi Lee,

It's not a full review (especially since it's getting late and Brooks  
said he planned to do one), but here are a few questions and remarks...

> 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).

> 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.

> @@ -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.

> +  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)


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.


Thanks,
FX