This is the mail archive of the mailing list for the GCC project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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

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

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]