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

[Brooks Moses <brooks dot moses at codesourcery dot com>]

Lee Millward wrote:
> Bootstrapped and regression tested with no new failures on
> i686-pc-linux-gnu, ok to apply?

Almost.  :)  There's a "gcc_assert (curr_arg == nargs)" that needs to be 
added after the main loop in gfc_conv_intrinsic_function_args, some 
small semantic things that need to be corrected, and a number of trivial 
whitespace problems.  Detailed comments are below.

I believe this should be OK once those are corrected (and retested to 
make sure the new assert doesn't find anything), assuming FX is happy 
with your answers to his questions.  However, I'd like to give the 
revised patch a final once-over before you commit it.

(Also, that will give people a chance to comment on the discussion of 
why this is useful.)

- Brooks


---------------------------------------------------------------

> Index: trans-intrinsic.c
> ===================================================================
> --- trans-intrinsic.c	(revision 124499)
> +++ trans-intrinsic.c	(working copy)
> @@ -164,19 +164,17 @@ real_compnt_info;
>  enum rounding_mode { RND_ROUND, RND_TRUNC, RND_CEIL, RND_FLOOR };
>  
>  /* Evaluate the arguments to an intrinsic function.  */
> -/* FIXME: This function and its callers should be rewritten so that it's
> -   not necessary to cons up a list to hold the arguments.  */
>  
> -static tree
> -gfc_conv_intrinsic_function_args (gfc_se * se, gfc_expr * expr)
> +static void
> +gfc_conv_intrinsic_function_args (gfc_se * se, gfc_expr * expr,
> +				  tree *argarray, int nargs)

That should be *se and *expr, without spaces.

(Yeah, I know the rest of the file doesn't follow this rule.  It all 
needs to be fixed, and being consistent isn't actually useful.)

> @@ -195,7 +193,8 @@ gfc_conv_intrinsic_function_args (gfc_se
>  	{
>  	  gfc_conv_expr (&argse, e);
>  	  gfc_conv_string_parameter (&argse);
> -	  args = gfc_chainon_list (args, argse.string_length);
> +	  gcc_assert (curr_arg < nargs);
> +          argarray[curr_arg++] = argse.string_length;
>  	}

Your argarray[curr_arg++] line has tab damage.

> @@ -210,9 +209,34 @@ gfc_conv_intrinsic_function_args (gfc_se
>  
>        gfc_add_block_to_block (&se->pre, &argse.pre);
>        gfc_add_block_to_block (&se->post, &argse.post);
> -      args = gfc_chainon_list (args, argse.expr);
> +      gcc_assert (curr_arg < nargs);
> +      argarray[curr_arg++] = argse.expr;
>      }

There should be a "gcc_assert (curr_arg == nargs)" after this loop to 
catch cases where nargs is too large.

> -  return args;
> +}
> +
> +/* Count the number of actual arguments to the intrinsic function EXPR
> +   including any "hidden" string length arguments.  */
> +
> +static int
> +gfc_intrinsic_argument_list_length (gfc_expr * expr)

This should be "*expr".

> +{
> +  int n = 0;
> +  gfc_actual_arglist *actual;
> +  gfc_intrinsic_arg  *formal = expr->value.function.isym->formal;;

There is no need for formal in this function; it is never used.

> +
> +  for (actual = expr->value.function.actual; actual; actual = actual->next,
> +       formal = formal ? formal->next : NULL)

Similarly, the whole second line of this is irrelevant.

> +    {
> +      if(!actual->expr)
> +	continue;

This should have a copy of the "Skip omitted optional arguments" comment 
from the equivalent clause in gfc_conv_intrinsic_function_args.

> @@ -223,26 +247,25 @@ static void
>  gfc_conv_intrinsic_conversion (gfc_se * se, gfc_expr * expr)
>  {
>    tree type;
> -  tree arg;
> +  tree args[2];
>  
>    /* Evaluate the argument.  */
>    type = gfc_typenode_for_spec (&expr->ts);
>    gcc_assert (expr->value.function.actual->expr);
> -  arg = gfc_conv_intrinsic_function_args (se, expr);
> -  arg = TREE_VALUE (arg);
> +  gfc_conv_intrinsic_function_args (se, expr, args, 2);

As FX asked, why are there two arguments here?  That seems entirely wrong.

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

Same question here -- why two arguments?

> @@ -445,33 +467,32 @@ static void
>  gfc_conv_intrinsic_int (gfc_se * se, gfc_expr * expr, enum rounding_mode op)
>  {
>    tree type;
> -  tree arg;
> +  tree args[2];

And same here.

> @@ -666,9 +687,16 @@ gfc_conv_intrinsic_lib_function (gfc_se 
>      }
>  
>    /* Get the decl and generate the call.  */
> -  args = gfc_conv_intrinsic_function_args (se, expr);
> +  num_args = gfc_intrinsic_argument_list_length (expr);
> +  args = alloca (sizeof (tree) * num_args);
> +
> +  gfc_conv_intrinsic_function_args (se, expr, args, num_args);
>    fndecl = gfc_get_intrinsic_lib_fndecl (m, expr);
> -  se->expr = build_function_call_expr (fndecl, args);
> +  rettype = TREE_TYPE (TREE_TYPE (fndecl));
> +
> +  fndecl = build_addr (fndecl, current_function_decl);
> +  se->expr = build_call_array (rettype, fndecl, num_args, args);
> +  se->expr = se->expr;

This last line is erroneous.

> @@ -946,23 +971,27 @@ gfc_conv_intrinsic_abs (gfc_se * se, gfc
>  
>  /* Create a complex value from one or two real components.  */
>  
> +

This extra blank line is erroneous.

> @@ -971,6 +1000,7 @@ gfc_conv_intrinsic_cmplx (gfc_se * se, g
>    se->expr = fold_build2 (COMPLEX_EXPR, type, real, imag);
>  }
>  
> +

This one likewise.

> @@ -1029,18 +1056,17 @@ gfc_conv_intrinsic_mod (gfc_se * se, gfc
>        /* Use it if it exists.  */
>        if (n != END_BUILTINS)
>  	{
> -	  tmp = built_in_decls[n];
> -	  se->expr = build_function_call_expr (tmp, arg);
> +  	  tmp = build_addr (built_in_decls[n], current_function_decl);
> +	  se->expr = build_call_array (TREE_TYPE (TREE_TYPE (built_in_decls[n])), 

There's an erroneous space at the end of this line.

> @@ -1109,19 +1135,16 @@ gfc_conv_intrinsic_mod (gfc_se * se, gfc
>  static void
>  gfc_conv_intrinsic_dim (gfc_se * se, gfc_expr * expr)
>  {
> -  tree arg;
> -  tree arg2;
>    tree val;
>    tree tmp;
>    tree type;
>    tree zero;
> +  tree args[2];
> +  

This blank line has two spaces in it, which shouldn't be there.

> @@ -1209,19 +1229,16 @@ gfc_conv_intrinsic_present (gfc_se * se,
>  static void
>  gfc_conv_intrinsic_dprod (gfc_se * se, gfc_expr * expr)
>  {
> -  tree arg;
> -  tree arg2;
>    tree type;
> -
> -  arg = gfc_conv_intrinsic_function_args (se, expr);
> -  arg2 = TREE_VALUE (TREE_CHAIN (arg));
> -  arg = TREE_VALUE (arg);
> +  tree args[2];
> + 

And this one has one space in it, which likewise should go.

> @@ -1255,21 +1271,27 @@ gfc_conv_intrinsic_ctime (gfc_se * se, g
[...]
> +
> +  fndecl = build_addr (gfor_fndecl_ctime, current_function_decl);
> +  tmp = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_ctime)), 

There's an erroneous space at the end of this line.

> @@ -1290,21 +1312,27 @@ gfc_conv_intrinsic_fdate (gfc_se * se, g
[...]
> +  args[1] = build_fold_addr_expr (len);
> +
> +  fndecl = build_addr (gfor_fndecl_fdate, current_function_decl);
> +  tmp = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_fdate)), 

There's an erroneous space at the end of this line.

> @@ -1327,21 +1355,27 @@ gfc_conv_intrinsic_ttynam (gfc_se * se, 
[...]
> +  gfc_conv_intrinsic_function_args (se, expr, &args[2], num_args - 2);
> +  args[0] = build_fold_addr_expr (var);
> +  args[1] = build_fold_addr_expr (len);
> +
> +  fndecl = build_addr (gfor_fndecl_ttynam, current_function_decl);
> +  tmp = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_ttynam)), 

There's an erroneous space at the end of this line.

> @@ -1381,13 +1415,18 @@ gfc_conv_intrinsic_minmax (gfc_se * se, 
>    tree val;
>    tree thencase;
>    tree elsecase;
> -  tree arg;
>    tree type;
> +  tree *args;
> +  int num_args;
> +  int i;
[...]
> @@ -1396,9 +1435,9 @@ gfc_conv_intrinsic_minmax (gfc_se * se, 
>  
>    mvar = gfc_create_var (type, "M");
>    elsecase = build2_v (MODIFY_EXPR, mvar, limit);
> -  for (arg = TREE_CHAIN (arg); arg != NULL_TREE; arg = TREE_CHAIN (arg))
> +  for (i = 0; i < num_args; i++)

"i" is only used in the loop, and thus should be declared within the 
"for" construct.

> @@ -2433,38 +2446,37 @@ gfc_conv_intrinsic_ishft (gfc_se * se, g
>  static void
>  gfc_conv_intrinsic_ishftc (gfc_se * se, gfc_expr * expr)
[...]
> +  num_args = gfc_intrinsic_argument_list_length (expr);
> +  args = alloca (sizeof (tree) * num_args);
> +
> +  gfc_conv_intrinsic_function_args (se, expr, args, num_args);;

The ";;" at the end of that should be just ";".

[...]
> -	  tmp = convert (int4type, TREE_VALUE (arg));
> -	  TREE_VALUE (arg) = tmp;
> -	}
> +	args[0] = convert (int4type, args[0]);
> +	 

This blank line has a tab and a space in it, which should go.

> @@ -2596,44 +2606,45 @@ static void
>  gfc_conv_intrinsic_index (gfc_se * se, gfc_expr * expr)
[...]
>      }
>  
> -  se->expr = build_function_call_expr (gfor_fndecl_string_index, args);
> +  fndecl = build_addr (gfor_fndecl_string_index, current_function_decl);
> +  se->expr = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_string_index)), 

There's an erroneous space at the end of this line.

> @@ -3233,27 +3240,28 @@ static void
>  gfc_conv_intrinsic_scan (gfc_se * se, gfc_expr * expr)
[...]
> -      back = tree_cons (NULL_TREE, build_int_cst (logical4_type_node, 0),
> -			NULL_TREE);
> -      TREE_CHAIN (tmp) = back;
> -    }
> + 

This empty line has an erroneous space in it.

> +  if (num_args == 4)
> +      args[4] = build_int_cst (logical4_type_node, 0);

This is indented two spaces too far.

[...]
> -  se->expr = build_function_call_expr (gfor_fndecl_string_scan, args);
> +  fndecl = build_addr (gfor_fndecl_string_scan, current_function_decl);
> +  se->expr = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_string_scan)), 

There's an erroneous space at the end of this line.

> @@ -3266,27 +3274,26 @@ static void
>  gfc_conv_intrinsic_verify (gfc_se * se, gfc_expr * expr)
[...]
> +
> +  if (num_args == 4)
> +      args[4] = build_int_cst (logical4_type_node, 0);

The indentation on the if clause here is off by 2.

>    else
> -    {
> -      back = TREE_CHAIN (tmp);
> -      TREE_VALUE (back) = convert (logical4_type_node, TREE_VALUE (back));
> -    }
> +      args[4] = convert (logical4_type_node, args[4]);

This should get an assert that num_args == 5, like the "index" and 
"scan" implementations have.

> +
> +  fndecl = build_addr (gfor_fndecl_string_verify, current_function_decl);
> +  se->expr = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_string_verify)), 

There's an erroneous space at the end of this line.

> @@ -3342,23 +3348,27 @@ gfc_conv_intrinsic_trim (gfc_se * se, gf
[...]
> +
> +  fndecl = build_addr (gfor_fndecl_string_trim, current_function_decl);
> +  tmp = build_call_array (TREE_TYPE (TREE_TYPE (gfor_fndecl_string_trim)), 

There's an erroneous space at the end of this line.

> @@ -3378,17 +3388,16 @@ gfc_conv_intrinsic_trim (gfc_se * se, gf
>  static void
>  gfc_conv_intrinsic_repeat (gfc_se * se, gfc_expr * expr)
>  {
> -  tree args, ncopies, dest, dlen, src, slen, ncopies_type;
> +  tree args[3], ncopies, dest, dlen, src, slen, ncopies_type;
[...]
> +  gfc_conv_intrinsic_function_args (se, expr, args, 4);

You've got a mismatch here; this needs to be 3 as well.