Re: [PATCH][RFC] Add gimple_fold

[Diego Novillo <dnovillo at google dot com>]

On 03/18/2011 10:11 AM, Richard Guenther wrote:
> This tries to extend the previously posted CCP folding patch by
> introducing a generic interface for non-tree-building, GIMPLE SSA
> aware folding.  The low-level interface for folding regular
> operations is
>
> /* Fold the expression composed by *CODEP, TYPE and valueized operands
> *OP0P,
>     *OP1P and *OP2P (as required), using the VALUEIZE callback to valueize
>     SSA names that turn up during statement lookup and producing a result
>     of at most KIND.  The folding result will be stored in the operands.
>     GF_KIND_FAIL is returned if no folding was done.
>     If GF_KIND_CONST or GF_KIND_VAL is returned the folding result is
>     stored in *OP0P and is a is_gimple_min_invariant or a is_gimple_val.
>     If GF_KIND_STMT is returned the folding result is stored in *CODEP,
>     *OP0P, *OP1P and *OP2P (as required) and will compose a valid set of
>     operands for a gimple assignment.
>     If GF_KIND_COMPLEX is returned the folding result is stored in *OP0P
>     and will be an arbitrarily complex tree.  */
>
> enum gf_kind
> gimple_fold (enum tree_code *codep, tree type,
>               tree *op0p, tree *op1p, tree *op2p,
>               tree (*valueize)(tree), enum gf_kind kind)
>
> which would allow existing gimple-level foldings to build ontop of this
> (including fold_stmt and friends and the special CCP foldings).
>
> The current implementation is quite lame, just dispatching to fold,
> but I plan to integrate it with the CCP and fold_stmt foldings before
> considering to merge it (eventually producing some sort of statistics
> of what the fold path catches that the gimple path does not).
>
> The low-level interface needs to be amended by one for builtin calls
> where I'm not sure if it is going to take decomposed operands
> or just a gimple statement (I think we have at most 3 operands to
> any builtin we can fold in some way).
>
> This again is just a RFC (even though the patch "works" as far as
> plugging into the SCCVN binary operation simplification).

I like this.  In terms of the interface, what do you think about this 
always returning an SSA name or (may be) the statement that defines it. 
 Although, if this is just the low-level interface, then we can build a 
wrapper that calls it and returns a statement.

Some questions and nits below:

> + enum gf_kind {
> +     GF_KIND_FAIL = 0,
> +     GF_KIND_CONST,
> +     GF_KIND_VAL,
> +     GF_KIND_STMT,
> +     GF_KIND_COMPLEX
> + };

These will need documentation.  The values are also sorted by level of 
complexity, right?

Should we not bother with GF_KIND_COMPLEX?  If we are going to return an 
arbitrarily complex tree, we might as well return GF_KIND_FAIL, I think. 
 Are you implementing GF_KIND_COMPLEX as a workaround or is this the 
expected behaviour?

For instance, if folding ends up producing a complex tree, would it need 
to be gimplified?  Maybe we could generate the gimplified statement and 
return the SSA name on the LHS?  Though I'm not sure I like even this.

> +
> +
> + static tree
> + build_valueized_tree_from_def (tree name, tree (*valueize)(tree))
> + {

Needs comment.

> +   gimple def_stmt = SSA_NAME_DEF_STMT (name);
> +   tree op0, op1, op2;
> +
> +   if (!is_gimple_assign (def_stmt)
> +       || gimple_vuse (def_stmt))
> +     return name;
> +
> +   switch (gimple_assign_rhs_class (def_stmt))
> +     {
> +     case GIMPLE_SINGLE_RHS:
> +       return gimple_assign_rhs1 (def_stmt);
> +
> +     case GIMPLE_TERNARY_RHS:
> +       op2 = gimple_assign_rhs3 (def_stmt);
> +       if (TREE_CODE (op2) == SSA_NAME
> + 	&&  valueize)
> + 	op2 = (*valueize) (op2);
> +
> +     case GIMPLE_BINARY_RHS:
> +       op1 = gimple_assign_rhs2 (def_stmt);
> +       if (TREE_CODE (op1) == SSA_NAME
> + 	&&  valueize)
> + 	op1 = (*valueize) (op1);
> +

/* Fallthru  */

> +     case GIMPLE_UNARY_RHS:
> +       op0 = gimple_assign_rhs1 (def_stmt);
> +       if (TREE_CODE (op0) == SSA_NAME
> + 	&&  valueize)
> + 	op0 = (*valueize) (op0);
> +       break;
> +
> +     default:;
> +     }
> +
> +   switch (gimple_assign_rhs_class (def_stmt))
> +     {
> +     case GIMPLE_UNARY_RHS:
> +       return fold_build1 (gimple_assign_rhs_code (def_stmt),
> + 			  TREE_TYPE (name), op0);
> +     case GIMPLE_BINARY_RHS:
> +       return fold_build2 (gimple_assign_rhs_code (def_stmt),
> + 			  TREE_TYPE (name), op0, op1);
> +     case GIMPLE_TERNARY_RHS:
> +       return fold_build3 (gimple_assign_rhs_code (def_stmt),
> + 			  TREE_TYPE (name), op0, op1, op2);
> +     default:;
> +     }
> +
> +   return name;
> + }
> +
> + static enum gf_kind
> + const_or_val (tree res, enum gf_kind kind)
> + {

Needs comment.


Diego.