vec_cond_expr adjustments

Hello,

I have been experimenting with generating VEC_COND_EXPR from the 
front-end, and these are just a couple things I noticed.

1) optabs.c requires that the first argument of vec_cond_expr be a 
comparison, but verify_gimple_assign_ternary only checks 
is_gimple_condexpr, like for COND_EXPR. In the long term, it seems better 
to also allow ssa_name and vector_cst (thus match the is_gimple_condexpr 
condition), but for now I just want to know early if I created an invalid 
vec_cond_expr.

2) a little refactoring of the code to find a suitable vector type for 
comparison results, and one more place where it should be used (no 
testcase yet because I don't know if that path can be taken without 
front-end changes first). I did wonder, for tree-ssa-forwprop, about using 
directly TREE_TYPE (cond) without truth_type_for.

Hmm, now I am wondering whether I should have waited until I had front-end 
vec_cond_expr support to submit everything at once...

2012-09-27  Marc Glisse  <marc.glisse@inria.fr>

 	* tree-cfg.c (verify_gimple_assign_ternary): Stricter check on
 	first argument of VEC_COND_EXPR.
 	* tree.c (truth_type_for): New function.
 	* tree.h (truth_type_for): Declare.
 	* gimple-fold.c (and_comparisons_1): Call it.
 	(or_comparisons_1): Likewise.
 	* tree-ssa-forwprop.c (forward_propagate_into_cond): Likewise.

On Fri, Sep 28, 2012 at 12:42 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
> Hello,
>
> I have been experimenting with generating VEC_COND_EXPR from the front-end,
> and these are just a couple things I noticed.
>
> 1) optabs.c requires that the first argument of vec_cond_expr be a
> comparison, but verify_gimple_assign_ternary only checks is_gimple_condexpr,
> like for COND_EXPR. In the long term, it seems better to also allow ssa_name
> and vector_cst (thus match the is_gimple_condexpr condition), but for now I
> just want to know early if I created an invalid vec_cond_expr.

optabs should be fixed instead, an is_gimple_val condition is implicitely
val != 0.

> 2) a little refactoring of the code to find a suitable vector type for
> comparison results, and one more place where it should be used (no testcase
> yet because I don't know if that path can be taken without front-end changes
> first).

Yes, it looks fine to me.

> I did wonder, for tree-ssa-forwprop, about using directly TREE_TYPE
> (cond) without truth_type_for.

Yes, that should work.

> Hmm, now I am wondering whether I should have waited until I had front-end
> vec_cond_expr support to submit everything at once...

;)

The tree.[ch] and gimple-fold.c hunks are ok if tested properly, the
tree-ssa-forwprop.c idea of using TREE_TYPE (cond), too.

I don't like the tree-cfg.c change, instead re-factor optabs.c to
get a decomposed cond for vector_compare_rtx and appropriately
"decompose" a non-comparison-class cond in expand_vec_cond_expr.

If we for example have

 predicate = a < b;
 x = predicate ? d : e;
 y = predicate ? f : g;

we ideally want to re-use the predicate computation on targets where
that would be optimal (and combine should be able to recover the
case where it is not).

Thanks,
Richard.

> 2012-09-27  Marc Glisse  <marc.glisse@inria.fr>
>
>         * tree-cfg.c (verify_gimple_assign_ternary): Stricter check on
>         first argument of VEC_COND_EXPR.
>         * tree.c (truth_type_for): New function.
>         * tree.h (truth_type_for): Declare.
>         * gimple-fold.c (and_comparisons_1): Call it.
>         (or_comparisons_1): Likewise.
>         * tree-ssa-forwprop.c (forward_propagate_into_cond): Likewise.
>
> --
> Marc Glisse
> Index: gcc/tree-ssa-forwprop.c
> ===================================================================
> --- gcc/tree-ssa-forwprop.c     (revision 191810)
> +++ gcc/tree-ssa-forwprop.c     (working copy)
> @@ -549,21 +549,22 @@ static bool
>  forward_propagate_into_cond (gimple_stmt_iterator *gsi_p)
>  {
>    gimple stmt = gsi_stmt (*gsi_p);
>    tree tmp = NULL_TREE;
>    tree cond = gimple_assign_rhs1 (stmt);
>    bool swap = false;
>
>    /* We can do tree combining on SSA_NAME and comparison expressions.  */
>    if (COMPARISON_CLASS_P (cond))
>      tmp = forward_propagate_into_comparison_1 (stmt, TREE_CODE (cond),
> -                                              boolean_type_node,
> +                                              truth_type_for
> +                                                (TREE_TYPE (cond)),
>                                                TREE_OPERAND (cond, 0),
>                                                TREE_OPERAND (cond, 1));
>    else if (TREE_CODE (cond) == SSA_NAME)
>      {
>        enum tree_code code;
>        tree name = cond;
>        gimple def_stmt = get_prop_source_stmt (name, true, NULL);
>        if (!def_stmt || !can_propagate_from (def_stmt))
>         return 0;
>
> Index: gcc/tree-cfg.c
> ===================================================================
> --- gcc/tree-cfg.c      (revision 191810)
> +++ gcc/tree-cfg.c      (working copy)
> @@ -3758,22 +3758,24 @@ verify_gimple_assign_ternary (gimple stm
>    tree rhs2_type = TREE_TYPE (rhs2);
>    tree rhs3 = gimple_assign_rhs3 (stmt);
>    tree rhs3_type = TREE_TYPE (rhs3);
>
>    if (!is_gimple_reg (lhs))
>      {
>        error ("non-register as LHS of ternary operation");
>        return true;
>      }
>
> -  if (((rhs_code == VEC_COND_EXPR || rhs_code == COND_EXPR)
> -       ? !is_gimple_condexpr (rhs1) : !is_gimple_val (rhs1))
> +  if (((rhs_code == COND_EXPR) ? !is_gimple_condexpr (rhs1)
> +       : (rhs_code == VEC_COND_EXPR) ? (!is_gimple_condexpr (rhs1)
> +                                       || is_gimple_val (rhs1))
> +       : !is_gimple_val (rhs1))
>        || !is_gimple_val (rhs2)
>        || !is_gimple_val (rhs3))
>      {
>        error ("invalid operands in ternary operation");
>        return true;
>      }
>
>    /* First handle operations that involve different types.  */
>    switch (rhs_code)
>      {
> Index: gcc/gimple-fold.c
> ===================================================================
> --- gcc/gimple-fold.c   (revision 191810)
> +++ gcc/gimple-fold.c   (working copy)
> @@ -23,21 +23,20 @@ along with GCC; see the file COPYING3.
>  #include "coretypes.h"
>  #include "tm.h"
>  #include "tree.h"
>  #include "flags.h"
>  #include "function.h"
>  #include "dumpfile.h"
>  #include "tree-flow.h"
>  #include "tree-ssa-propagate.h"
>  #include "target.h"
>  #include "gimple-fold.h"
> -#include "langhooks.h"
>
>  /* Return true when DECL can be referenced from current unit.
>     FROM_DECL (if non-null) specify constructor of variable DECL was taken
> from.
>     We can get declarations that are not possible to reference for various
>     reasons:
>
>       1) When analyzing C++ virtual tables.
>         C++ virtual tables do have known constructors even
>         when they are keyed to other compilation unit.
>         Those tables can contain pointers to methods and vars
> @@ -1686,29 +1685,21 @@ and_var_with_comparison_1 (gimple stmt,
>     (OP1A CODE1 OP1B) and (OP2A CODE2 OP2B), respectively.
>     If this can be done without constructing an intermediate value,
>     return the resulting tree; otherwise NULL_TREE is returned.
>     This function is deliberately asymmetric as it recurses on SSA_DEFs
>     in the first comparison but not the second.  */
>
>  static tree
>  and_comparisons_1 (enum tree_code code1, tree op1a, tree op1b,
>                    enum tree_code code2, tree op2a, tree op2b)
>  {
> -  tree truth_type = boolean_type_node;
> -  if (TREE_CODE (TREE_TYPE (op1a)) == VECTOR_TYPE)
> -    {
> -      tree vec_type = TREE_TYPE (op1a);
> -      tree elem = lang_hooks.types.type_for_size
> -       (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (vec_type))), 0);
> -      truth_type = build_opaque_vector_type (elem,
> -                                            TYPE_VECTOR_SUBPARTS
> (vec_type));
> -    }
> +  tree truth_type = truth_type_for (TREE_TYPE (op1a));
>
>    /* First check for ((x CODE1 y) AND (x CODE2 y)).  */
>    if (operand_equal_p (op1a, op2a, 0)
>        && operand_equal_p (op1b, op2b, 0))
>      {
>        /* Result will be either NULL_TREE, or a combined comparison.  */
>        tree t = combine_comparisons (UNKNOWN_LOCATION,
>                                     TRUTH_ANDIF_EXPR, code1, code2,
>                                     truth_type, op1a, op1b);
>        if (t)
> @@ -2158,29 +2149,21 @@ or_var_with_comparison_1 (gimple stmt,
>     (OP1A CODE1 OP1B) and (OP2A CODE2 OP2B), respectively.
>     If this can be done without constructing an intermediate value,
>     return the resulting tree; otherwise NULL_TREE is returned.
>     This function is deliberately asymmetric as it recurses on SSA_DEFs
>     in the first comparison but not the second.  */
>
>  static tree
>  or_comparisons_1 (enum tree_code code1, tree op1a, tree op1b,
>                   enum tree_code code2, tree op2a, tree op2b)
>  {
> -  tree truth_type = boolean_type_node;
> -  if (TREE_CODE (TREE_TYPE (op1a)) == VECTOR_TYPE)
> -    {
> -      tree vec_type = TREE_TYPE (op1a);
> -      tree elem = lang_hooks.types.type_for_size
> -       (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (vec_type))), 0);
> -      truth_type = build_opaque_vector_type (elem,
> -                                            TYPE_VECTOR_SUBPARTS
> (vec_type));
> -    }
> +  tree truth_type = truth_type_for (TREE_TYPE (op1a));
>
>    /* First check for ((x CODE1 y) OR (x CODE2 y)).  */
>    if (operand_equal_p (op1a, op2a, 0)
>        && operand_equal_p (op1b, op2b, 0))
>      {
>        /* Result will be either NULL_TREE, or a combined comparison.  */
>        tree t = combine_comparisons (UNKNOWN_LOCATION,
>                                     TRUTH_ORIF_EXPR, code1, code2,
>                                     truth_type, op1a, op1b);
>        if (t)
> Index: gcc/tree.c
> ===================================================================
> --- gcc/tree.c  (revision 191810)
> +++ gcc/tree.c  (working copy)
> @@ -10243,20 +10243,36 @@ unsigned_type_for (tree type)
>  /* If TYPE is an integral or pointer type, return an integer type with
>     the same precision which is signed, or itself if TYPE is already a
>     signed integer type.  */
>
>  tree
>  signed_type_for (tree type)
>  {
>    return signed_or_unsigned_type_for (0, type);
>  }
>
> +/* If TYPE is a vector type, return a signed integer vector type with the
> +   same width and number of subparts. Otherwise return boolean_type_node.
> */
> +
> +tree
> +truth_type_for (tree type)
> +{
> +  if (TREE_CODE (type) == VECTOR_TYPE)
> +    {
> +      tree elem = lang_hooks.types.type_for_size
> +        (GET_MODE_BITSIZE (TYPE_MODE (TREE_TYPE (type))), 0);
> +      return build_opaque_vector_type (elem, TYPE_VECTOR_SUBPARTS (type));
> +    }
> +  else
> +    return boolean_type_node;
> +}
> +
>  /* Returns the largest value obtainable by casting something in INNER type
> to
>     OUTER type.  */
>
>  tree
>  upper_bound_in_type (tree outer, tree inner)
>  {
>    double_int high;
>    unsigned int det = 0;
>    unsigned oprec = TYPE_PRECISION (outer);
>    unsigned iprec = TYPE_PRECISION (inner);
> Index: gcc/tree.h
> ===================================================================
> --- gcc/tree.h  (revision 191810)
> +++ gcc/tree.h  (working copy)
> @@ -4762,20 +4762,21 @@ extern tree build_call_valist (tree, tre
>  extern tree build_call_array_loc (location_t, tree, tree, int, const tree
> *);
>  extern tree build_call_vec (tree, tree, VEC(tree,gc) *);
>
>  /* Construct various nodes representing data types.  */
>
>  extern tree make_signed_type (int);
>  extern tree make_unsigned_type (int);
>  extern tree signed_or_unsigned_type_for (int, tree);
>  extern tree signed_type_for (tree);
>  extern tree unsigned_type_for (tree);
> +extern tree truth_type_for (tree);
>  extern void initialize_sizetypes (void);
>  extern void fixup_unsigned_type (tree);
>  extern tree build_pointer_type_for_mode (tree, enum machine_mode, bool);
>  extern tree build_pointer_type (tree);
>  extern tree build_reference_type_for_mode (tree, enum machine_mode, bool);
>  extern tree build_reference_type (tree);
>  extern tree build_vector_type_for_mode (tree, enum machine_mode);
>  extern tree build_vector_type (tree innertype, int nunits);
>  extern tree build_opaque_vector_type (tree innertype, int nunits);
>  extern tree build_type_no_quals (tree);
>

On Fri, 28 Sep 2012, Richard Guenther wrote:

> On Fri, Sep 28, 2012 at 12:42 AM, Marc Glisse <marc.glisse@inria.fr> wrote:
>> Hello,
>>
>> I have been experimenting with generating VEC_COND_EXPR from the front-end,
>> and these are just a couple things I noticed.
>>
>> 1) optabs.c requires that the first argument of vec_cond_expr be a
>> comparison, but verify_gimple_assign_ternary only checks is_gimple_condexpr,
>> like for COND_EXPR. In the long term, it seems better to also allow ssa_name
>> and vector_cst (thus match the is_gimple_condexpr condition), but for now I
>> just want to know early if I created an invalid vec_cond_expr.
>
> optabs should be fixed instead, an is_gimple_val condition is implicitely
> val != 0.

For vectors, I think it should be val < 0 (with an appropriate cast of val 
to a signed integer vector type if necessary). Or (val & highbit) != 0, 
but that's longer.

> The tree.[ch] and gimple-fold.c hunks are ok if tested properly, the
> tree-ssa-forwprop.c idea of using TREE_TYPE (cond), too.

Ok, I will retest that way.

> I don't like the tree-cfg.c change, instead re-factor optabs.c to
> get a decomposed cond for vector_compare_rtx and appropriately
> "decompose" a non-comparison-class cond in expand_vec_cond_expr.

So vector_compare_rtx will take as arguments rcode, t_op0, t_op1 instead 
of cond. And in expand_vec_cond_expr, if I have a condition, I pass its 
elements to vector_compare_rtx, and otherwise I use 0 and the code for 
LT_EXPR as the other arguments.

> If we for example have
>
> predicate = a < b;
> x = predicate ? d : e;
> y = predicate ? f : g;
>
> we ideally want to re-use the predicate computation on targets where
> that would be optimal (and combine should be able to recover the
> case where it is not).

That I don't understand. The vcond instruction implemented by targets 
takes as arguments d, e, cmp, a, b and emits the comparison itself. I 
don't see how I can avoid sending to the targets both (d,e,<,a,b) and 
(f,g,<,a,b). They will notice eventually that a<b is computed twice and 
remove one of the two, but I don't see how to do that in optabs.c. Or I 
can compute x = a < b, use x < 0 as the comparison passed to the targets, 
and expect targets (those for which it is true) to recognize that < 0 is 
useless in a vector condition (PR54700), or is useless on a comparison 
result.

Thanks for the comments,

On Fri, Sep 28, 2012 at 6:55 PM, Marc Glisse <marc.glisse@inria.fr> wrote:
> On Fri, 28 Sep 2012, Richard Guenther wrote:
>
>> On Fri, Sep 28, 2012 at 12:42 AM, Marc Glisse <marc.glisse@inria.fr>
>> wrote:
>>>
>>> Hello,
>>>
>>> I have been experimenting with generating VEC_COND_EXPR from the
>>> front-end,
>>> and these are just a couple things I noticed.
>>>
>>> 1) optabs.c requires that the first argument of vec_cond_expr be a
>>> comparison, but verify_gimple_assign_ternary only checks
>>> is_gimple_condexpr,
>>> like for COND_EXPR. In the long term, it seems better to also allow
>>> ssa_name
>>> and vector_cst (thus match the is_gimple_condexpr condition), but for now
>>> I
>>> just want to know early if I created an invalid vec_cond_expr.
>>
>>
>> optabs should be fixed instead, an is_gimple_val condition is implicitely
>> val != 0.
>
>
> For vectors, I think it should be val < 0 (with an appropriate cast of val
> to a signed integer vector type if necessary). Or (val & highbit) != 0, but
> that's longer.

I don't think so.  Throughout the compiler we generally assume false == 0
and anything else is true.  (yes, for FP there is STORE_FLAG_VALUE, but
it's scope is quite limited - if we want sth similar for vectors we'd have to
invent it).

>> The tree.[ch] and gimple-fold.c hunks are ok if tested properly, the
>> tree-ssa-forwprop.c idea of using TREE_TYPE (cond), too.
>
>
> Ok, I will retest that way.
>
>
>> I don't like the tree-cfg.c change, instead re-factor optabs.c to
>> get a decomposed cond for vector_compare_rtx and appropriately
>> "decompose" a non-comparison-class cond in expand_vec_cond_expr.
>
>
> So vector_compare_rtx will take as arguments rcode, t_op0, t_op1 instead of
> cond.

Yes.

> And in expand_vec_cond_expr, if I have a condition, I pass its
> elements to vector_compare_rtx, and otherwise I use 0 and the code for
> LT_EXPR as the other arguments.

Yes, but NE_EXPR and 0 (see above).

>
>> If we for example have
>>
>> predicate = a < b;
>> x = predicate ? d : e;
>> y = predicate ? f : g;
>>
>> we ideally want to re-use the predicate computation on targets where
>> that would be optimal (and combine should be able to recover the
>> case where it is not).
>
>
> That I don't understand. The vcond instruction implemented by targets takes
> as arguments d, e, cmp, a, b and emits the comparison itself. I don't see
> how I can avoid sending to the targets both (d,e,<,a,b) and (f,g,<,a,b).
> They will notice eventually that a<b is computed twice and remove one of the
> two, but I don't see how to do that in optabs.c. Or I can compute x = a < b,
> use x < 0 as the comparison passed to the targets, and expect targets (those
> for which it is true) to recognize that < 0 is useless in a vector condition
> (PR54700), or is useless on a comparison result.

But that's a limitation of how vcond works.  ISTR there is/was a vselect
instruction as well, taking a "mask" and two vectors to select from.  At least
that's how vcond works internally for some sub-targets.

Richard.

> Thanks for the comments,
>
> --
> Marc Glisse