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

[Lee Millward]

Hi Brooks,

On 5/8/07, Brooks Moses <brooks.moses@codesourcery.com> wrote:
> 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'll get that added in and retest.

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

Ok.

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

Previous versions of the patch didn't use formal for calculating the
number of arguments but this led to discrepencies in the number of
arguments returned from this function and the number expected by
gfc_conv_intrinsic_function_args. Since the two are meant to work in
conjunction with each other I adjusted this to make it behave the same
as gfc_conv_intrinsic_function_args since it was causing ICEs and
wrong-code problems in the testsuite.

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

Ok.

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

This and the other places you mention seems to be a leftover from an
earlier addition, I'll fix it in the revised version.

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

Hmm, not sure how that got in there, I'll get rid of it. :-(

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

Ok.

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

I thought I had added all those in but I obviously missed one. Will
fix in the next version.

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

Nicely caught :-)

Thanks for all your comments, I'll get everything fixed up and will
post a new version of the patch for review.

Cheers,
Lee.