Re: PATCH: CALL_EXPR representation part 1/9 (tree changes + documentation)

[Ian Lance Taylor <iant at google dot com>]

Sandra Loosemore <sandra@codesourcery.com> writes:

> + /* In a tcc_vl_exp node, operand 0 is an INT_CST node holding the operand
> +    length.  Note that we have to bypass the use of TREE_OPERAND to access
> +    that field to avoid infinite recursion in expanding the macros.  */
> + #define VL_EXP_OPERAND_LENGTH(NODE) \
> +   ((int)TREE_INT_CST_LOW (VL_EXP_CHECK (NODE)->exp.operands[0]))

The comment should explicitly say that the operand length includes the
length operand itself.  That is, the minimum valid length is 1.

>   /* In ordinary expression nodes.  */
> + #define TREE_OPERAND_LENGTH(NODE) tree_operand_length (NODE)

Do you have any timings of the compiler with your patch?  When
checking is enabled this is going to add a lot of function calls.  I
would like to see tree_operand_length be a static inline function in
tree.h, unless there is some reason not to do that.

> + /* CALL_EXPR accessors.
> +  */
> + #define CALL_EXPR_FN(NODE) TREE_OPERAND (VL_EXP_CHECK (NODE), 1)
> + #define CALL_EXPR_STATIC_CHAIN(NODE) TREE_OPERAND (VL_EXP_CHECK (NODE), 2)
> + #define CALL_EXPR_ARGS(NODE) call_expr_arglist (NODE)
> + #define CALL_EXPR_ARG0(NODE) TREE_OPERAND (VL_EXP_CHECK (NODE), 3)
> + #define CALL_EXPR_ARG1(NODE) TREE_OPERAND (VL_EXP_CHECK (NODE), 4)
> + #define CALL_EXPR_ARG2(NODE) TREE_OPERAND (VL_EXP_CHECK (NODE), 5)
> + #define CALL_EXPR_ARG(NODE, I) TREE_OPERAND (VL_EXP_CHECK (NODE), (I)+3)
> + #define call_expr_nargs(NODE) (VL_EXP_OPERAND_LENGTH(NODE) - 3)

These macros should all check that they are being applied to a
CALL_EXPR node.  Please change these to use the existing
CALL_EXPR_CHECK macro.  We may have other variable length tree codes
in the future, so checking VL_EXP_CHECK is not what we need here.

Please put spaces around the '+' in CALL_EXPR_ARG.

> + /* CALL_EXPR_ARGP returns a pointer to the argument vector for NODE.
> +    We can't use &CALL_EXPR_ARG0 (NODE) because that will complain if
> +    the argument count is zero when checking is enabled.  Instead, do
> +    the pointer arithmetic to advance past the 3 fixed operands in a
> +    CALL_EXPR.  That produces a valid pointer to just past the end of the
> +    argument array, even if it's not valid to dereference it.  */
> + #define CALL_EXPR_ARGP(NODE) \
> +   (&(TREE_OPERAND (VL_EXP_CHECK (NODE), 0)) + 3)

This should also use CALL_EXPR_CHECK.

> + extern tree build_nt_call_list (enum tree_code, tree, tree);

> + extern tree build_call_list (enum tree_code, tree, tree, tree);
> + extern tree build_call_nary (enum tree_code, tree, tree, int, ...);
> + extern tree build_call_valist (enum tree_code, tree, tree, int, va_list);
> + extern tree build_call_array (enum tree_code, tree, tree, int, tree*);

The names and current implementations of these functions imply that
they only build CALL_EXPRs.  In that case, they don't need to take a
tree_code parameter.  I'd like these to be reworked so that either
build general tcc_vl_exp nodes, or they just build CALL_EXPR nodes.

> + extern tree fold_build_call_list (enum tree_code, tree, tree, tree);

> + extern tree fold_build_call_list_initializer (enum tree_code, tree, tree, tree);

Here too: build a CALL_EXPR (and don't take a tree_code parameter), or
build a general list (and change the name of the function).


> *************** substitute_in_expr (tree exp, tree f, tr
> *** 2472,2477 ****
> --- 2502,2528 ----
>   	  }
>   	break;
>   
> +       case tcc_vl_exp:
> + 	{
> + 	  tree copy = NULL_TREE;
> + 	  int i;
> + 	  int n = TREE_OPERAND_LENGTH (exp);
> + 	  for (i = 0; i < n; i++)
> + 	    {
> + 	      tree op = TREE_OPERAND (exp, i);
> + 	      tree newop = SUBSTITUTE_IN_EXPR (op, f, r);
> + 	      if (newop != op)
> + 		{
> + 		  copy = copy_node (exp);
> + 		  TREE_OPERAND (copy, i) = newop;
> + 		}
> + 	    }
> + 	  if (copy)
> + 	    new = fold (copy);
> + 	  else
> + 	    return exp;
> + 	}

The loop over the operands can start at 1, since operand 0 is known to
be the length.


> *************** substitute_placeholder_in_expr (tree exp
> *** 2602,2607 ****
> --- 2655,2682 ----
>   	  }
>   	break;
>   
> +       case tcc_vl_exp:
> + 	{
> + 	  tree copy = NULL_TREE;
> + 	  int i;
> + 	  int n = TREE_OPERAND_LENGTH (exp);
> + 	  for (i = 0; i < n; i++)
> + 	    {
> + 	      tree op = TREE_OPERAND (exp, i);
> + 	      tree newop = SUBSTITUTE_PLACEHOLDER_IN_EXPR (op, obj);
> + 	      if (newop != op)
> + 		{
> + 		  if (!copy)
> + 		    copy = copy_node (exp);
> + 		  TREE_OPERAND (copy, i) = newop;
> + 		}
> + 	    }
> + 	  if (copy)
> + 	    return fold (copy);
> + 	  else
> + 	    return exp;
> + 	}

Here also.


> *************** build_omp_clause (enum omp_clause_code c
> *** 7305,7310 ****
> --- 7396,7554 ----
>     return t;
>   }
>   
> + /* Set various status flags when building a tcc_vl_exp object T.  */
> + 
> + static void
> + process_call_operands (tree t)
> + {
> +   bool side_effects;
> + 
> +   side_effects = TREE_SIDE_EFFECTS (t);
> +   if (!side_effects)
> +     {
> +       int i, n;
> +       n = TREE_OPERAND_LENGTH (t);
> +       for (i = 0; i < n; i++)
> + 	{
> + 	  tree op = TREE_OPERAND (t, i);
> + 	  if (op && TREE_SIDE_EFFECTS (op))
> + 	    {
> + 	      side_effects = 1;
> + 	      break;
> + 	    }
> + 	}

Here also.

> +     }
> +   if (TREE_CODE (t) == CALL_EXPR && !side_effects)
> +     {
> +       int i;
> + 
> +       /* Calls have side-effects, except those to const or
> + 	 pure functions.  */
> +       i = call_expr_flags (t);
> +       if (!(i & (ECF_CONST | ECF_PURE)))
> + 	side_effects = 1;
> +     }
> +   TREE_SIDE_EFFECTS (t) = side_effects;
> + }

This function is named process_call_operands, so you don't need to
check for CALL_EXPR.  Or, if you want to handle other node types
either, you should give the function a different name.

> + /* Build a tcc_vl_exp object with code CODE and room for LEN operands.  LEN
> +    includes the implicit operand count in TREE_OPERAND 0, and so must be >= 1.
> +    Except for the CODE and operand count field, other storage for the
> +    object is initialized to zeros.  */
> + 
> + tree
> + build_vl_exp_stat (enum tree_code code, int len MEM_STAT_DECL)
> + {
> +   tree t;
> +   int length = (len-1) * sizeof (tree) + sizeof (struct tree_exp);

Please add spaces around the '-'.

> + /* Build and return a TREE_LIST of arguments in the CALL_EXPR exp.
> +    FIXME: don't use this function.  It exists for compatibility with
> +    the old representation of CALL_EXPRs where a list was used to hold the
> +    arguments.  Places that currently extract the arglist from a CALL_EXPR
> +    ought to be rewritten to use the CALL_EXPR itself.  */
> + tree
> + call_expr_arglist (tree exp)
> + {
> +   tree arglist = NULL_TREE;
> +   int i;
> +   for (i = call_expr_nargs (exp) - 1; i >= 0; i--)
> +     arglist = tree_cons (NULL_TREE, CALL_EXPR_ARG (exp, i), arglist);
> +   return arglist;
> + }

Do you still need this function?  It's fine if you do need it.


Sorry for all the requested changes.  I think they should be fairly
mechanical.  Send another version of this patch when they are done.

Thanks.

Ian