| Message ID | CAFiYyc0ycip3timwZimk7Z0yuFTmckmYmfRu=WZPoTpxrN9d4w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Thu, Aug 25, 2011 at 11:09 AM, Richard Guenther <richard.guenther@gmail.com> wrote: > On Thu, Aug 25, 2011 at 8:20 AM, Artem Shinkarov > <artyom.shinkaroff@gmail.com> wrote: >> Here is a cleaned-up patch without the hook. Mostly it works in a way >> we discussed. >> >> So I think it is a right time to do something about vcond patterns, >> which would allow me to get rid of conversions that I need to put all >> over the code. >> >> Also at the moment the patch breaks lto frontend with a simple example: >> #define vector(elcount, type) \ >> __attribute__((vector_size((elcount)*sizeof(type)))) type >> >> int main (int argc, char *argv[]) { >> vector (4, float) f0; >> vector (4, float) f1; >> >> f0 = f1 != f0 >> ? (vector (4, float)){-1,-1,-1,-1} : (vector (4, float)){0,0,0,0}; >> >> return (int)f0[argc]; >> } >> >> test-lto.c:8:14: internal compiler error: in convert, at lto/lto-lang.c:1244 >> >> I looked into the file, the conversion function is defined as >> gcc_unreachable (). I am not very familiar with lto, so I don't really >> know what is the right way to treat the conversions. >> >> And I seriously need help with backend patterns. > > On the patch. > > The documentation needs review by a native english speaker, but here > are some factual comments: > > +In C vector comparison is supported within standard comparison operators: > > it should read 'In GNU C' here and everywhere else as this is a GNU > extension. > > The result of the > +comparison is a signed integer-type vector where the size of each > +element must be the same as the size of compared vectors element. > > The result type of the comparison is determined by the C frontend, > it isn't under control of the user. What you are implying here is > restrictions on vector assignments, which are documented elsewhere. > I'd just say > > 'The result of the comparison is a vector of the same width and number > of elements as the comparison operands with a signed integral element > type.' > > +In addition to the vector comparison C supports conditional expressions > > See above. > > +For the convenience condition in the vector conditional can be just a > +vector of signed integer type. > > 'of integer type.' I don't see a reason to disallow unsigned integers, > they can be equally well compared against zero. I'll have a final go on the documentation, it is untouched from the old patches. > Index: gcc/targhooks.h > =================================================================== > --- gcc/targhooks.h (revision 177665) > +++ gcc/targhooks.h (working copy) > @@ -86,6 +86,7 @@ extern int default_builtin_vectorization > extern tree default_builtin_reciprocal (unsigned int, bool, bool); > > extern bool default_builtin_vector_alignment_reachable (const_tree, bool); > + > extern bool > default_builtin_support_vector_misalignment (enum machine_mode mode, > const_tree, > > spurious whitespace change. Yes, thanks. > Index: gcc/optabs.c > =================================================================== > --- gcc/optabs.c (revision 177665) > +++ gcc/optabs.c (working copy) > @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type > ... > + else > + { > + rtx rtx_op0; > + rtx vec; > + > + rtx_op0 = expand_normal (op0); > + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX); > + vec = CONST0_RTX (mode); > + > + create_output_operand (&ops[0], target, mode); > + create_input_operand (&ops[1], rtx_op1, mode); > + create_input_operand (&ops[2], rtx_op2, mode); > + create_input_operand (&ops[3], comparison, mode); > + create_input_operand (&ops[4], rtx_op0, mode); > + create_input_operand (&ops[5], vec, mode); > > this still builds the fake(?) != comparison, but as you said you need help > with the .md part if we want to use a machine specific pattern for this > case (which we eventually want, for the sake of using XOP vcond). Yes, I am waiting for it. This is the only way at the moment to make sure that in m = a > b; r = m ? c : d; m in the vcond is not transformed into the m != 0. > Index: gcc/target.h > =================================================================== > --- gcc/target.h (revision 177665) > +++ gcc/target.h (working copy) > @@ -51,6 +51,7 @@ > #define GCC_TARGET_H > > #include "insn-modes.h" > +#include "gimple.h" > > #ifdef ENABLE_CHECKING > > spurious change. Old stuff, fixed. > @@ -9073,26 +9082,28 @@ fold_comparison (location_t loc, enum tr > floating-point, we can only do some of these simplifications.) */ > if (operand_equal_p (arg0, arg1, 0)) > { > + tree arg0_type = TREE_TYPE (arg0); > + > switch (code) > { > case EQ_EXPR: > - if (! FLOAT_TYPE_P (TREE_TYPE (arg0)) > - || ! HONOR_NANS (TYPE_MODE (TREE_TYPE (arg0)))) > + if (! FLOAT_TYPE_P (arg0_type) > + || ! HONOR_NANS (TYPE_MODE (arg0_type))) > ... Ok. > > Likewise. > > @@ -8440,6 +8440,37 @@ expand_expr_real_2 (sepops ops, rtx targ > case UNGE_EXPR: > case UNEQ_EXPR: > case LTGT_EXPR: > + if (TREE_CODE (ops->type) == VECTOR_TYPE) > + { > + enum tree_code code = ops->code; > + tree arg0 = ops->op0; > + tree arg1 = ops->op1; > > move this code to do_store_flag (we really store a flag value). It should > also simply do what expand_vec_cond_expr does, probably simply > calling that with the {-1,...} {0,...} extra args should work. I started to do that, but the code in do_store_flag is completely different from what I am doing, and it looks confusing. I just call expand_vec_cond_expr and that is it. I can write a separate function, but the code is quite small. > > As for the still required conversions, you should be able to delay those > from the C frontend (and here) to expand_vec_cond_expr by, after > expanding op1 and op2, wrapping a subreg around it with a proper mode > (using convert_mode (GET_MODE (comparison), rtx_op1)) should work), > and then convert the result back to the original mode. > > I'll leave the C frontend pieces of the patch for review by Joseph, but Conversions are there until we fix the backend. When backend will be able to digest f0 > f1 ? int0 : int1, all the conversions will go away. > +static tree > +fold_build_vec_cond_expr (tree ifexp, tree op1, tree op2) > > is missing a function comment. fixed. > +static tree > +do_compare (gimple_stmt_iterator *gsi, tree inner_type, tree a, tree b, > + tree bitpos, tree bitsize, enum tree_code code) > +{ > + tree cond; > + tree comp_type; > + > + a = tree_vec_extract (gsi, inner_type, a, bitsize, bitpos); > + b = tree_vec_extract (gsi, inner_type, b, bitsize, bitpos); > + > + comp_type = lang_hooks.types.type_for_size (TYPE_PRECISION (inner_type), 0); > + > > Use > > comp_type = build_nonstandard_integer_type (TYPE_PRECISION (inner_type), 0); > > instead. But I think you don't want to use TYPE_PRECISION on > FP types. Instead you want a signed integer type of the same (mode) > size as the vector element type, thus > > comp_type = build_nonstandard_integer_type (GET_MODE_BITSIZE > (TYPE_MODE (inner_type)), 0); Hm, I thought that at this stage we don't wan to know anything about modes. I mean here I am really building the same integer type as the operands of the comparison have. But I can use MODE_BITSIZE as well, I don't think that it could happen that the size of the mode is different from the size of the type. Or could it? > + cond = gimplify_build2 (gsi, code, comp_type, a, b); > > the result type of a comparison is boolean_type_node, not comp_type. > > + cond = gimplify_build2 (gsi, code, comp_type, a, b); > + return gimplify_build3 (gsi, COND_EXPR, comp_type, cond, > + build_int_cst (comp_type, -1), > + build_int_cst (comp_type, 0)); > > writing this as > > + return gimplify_build3 (gsi, COND_EXPR, comp_type, > fold_build2 (code, boolean_type_node, a, b), > + build_int_cst (comp_type, -1), > + build_int_cst (comp_type, 0)); > > will get the gimplifier a better chance at simplifcation. > > + if (! expand_vec_cond_expr_p (type, TYPE_MODE (type))) > > I think we are expecting the scalar type and the vector mode here > from looking at the single existing caller. It probably doesn't make > a difference (we only check TYPE_UNSIGNED of it, which should > also work for vector types), but let's be consistent. Thus, Ok. > if (! expand_vec_cond_expr_p (TREE_TYPE (type), TYPE_MODE (type))) > > + if (! expand_vec_cond_expr_p (type, TYPE_MODE (type))) > + t = expand_vector_piecewise (gsi, do_compare, type, > + TREE_TYPE (TREE_TYPE (op0)), op0, op1, code); > + else > + t = gimplify_build2 (gsi, code, type, op0, op1); > > the else case looks odd. Why re-build a stmt that already exists? > Simply return NULL_TREE instead? I can adjust. The reason it is written that way is that expand_vector_operations_1 is using the result of the function to update rhs. > +static tree > +expand_vec_cond_expr_piecewise (gimple_stmt_iterator *gsi, tree exp) > +{ > ... > + /* Expand vector condition inside of VEC_COND_EXPR. */ > + if (! expand_vec_cond_expr_p (TREE_TYPE (cond), > + TYPE_MODE (TREE_TYPE (cond)))) > + { > ... > + new_rhs = expand_vector_piecewise (gsi, do_compare, > + TREE_TYPE (cond), > + TREE_TYPE (TREE_TYPE (op1)), > + op0, op1, TREE_CODE (cond)); > > I'm not sure it is beneficial to expand a < b ? v0 : v1 to > > tem = { a[0] < b[0] ? -1 : 0, ... } > v0 & tem | v1 & ~tem; > > instead of > > { a[0] < b[0] ? v0[0] : v1[0], ... } > > even if the bitwise operations could be carried out using vectors. > It's definitely beneficial to do the first if the CPU can create the > bitmask. > o_O I thought you always wanted to do (m & v0) | (~m & v1). Do you want to have two cases of the expansion then -- when we have mask available and when we don't? But it is really unlikely that we can get the mask, but cannot get vcond. Because condition is actually vcond. So once again -- do we always expand to {a[0] > b[0] ? v[0] : c[0], ...}? > + /* Run vecower on the expresisons we have introduced. */ > + for (; gsi_tmp.ptr != gsi->ptr; gsi_next (&gsi_tmp)) > + expand_vector_operations_1 (&gsi_tmp); > > do not use gsi.ptr directly, use gsi_stmt (gsi_tm) != gsi_stmt (gsi) > > +static bool > +is_vector_comparison (gimple_stmt_iterator *gsi, tree expr) > +{ > > This function is lacking a comment. > > @@ -450,11 +637,41 @@ expand_vector_operations_1 (gimple_stmt_ > ... > + /* Try to get rid from the useless vector comparison > + x != {0,0,...} which is inserted by the typechecker. */ > + if (COMPARISON_CLASS_P (cond) && TREE_CODE (cond) == NE_EXPR) > > how and why? You simply drop that comparison - that doesn't look > correct. And in fact TREE_OPERAND (cond, 0) will never be a > comparison - that wouldn't be valid gimple. Please leave this > optimization to SSA based forward propagation (I can help you here > once the patch is in). No-no-no. This is the second part of avoiding m = a > b; r = m ? v0 : v1; to prevent m expansion to m != {0}. I do not _simply_ drop the comparison. I drop it only if is_vector_comparison returned true. It means that we can never get into the situation that we are dropping actually a comparison inserted by the user. But what I really want to achieve here is to drop the comparison that the frontend inserts every time when it sees an expression there. As I said earlier, tree forward propagation kicks only using -On, and I would really like to make sure that I can get rid of useless != {0} at any level. > > + if (expand_vec_cond_expr_p (TREE_TYPE (exp), > + TYPE_MODE (TREE_TYPE (exp)))) > + { > + update_stmt (gsi_stmt (*gsi)); > + return; > > no need to update the stmt when you do nothing. > > + new_rhs = expand_vec_cond_expr_piecewise (gsi, exp); > + gimple_assign_set_rhs_from_tree (gsi, new_rhs); > + update_stmt (gsi_stmt (*gsi)); > + } > > missing return;, just for clarity that you are done here. Ok. > You don't do anything for comparisons here, in case they are split > away from the VEC_COND_EXPR by the gimplifier. But if the > target doesn't support VEC_COND_EXPRs we have to lower them. > I suggest checking your testcases on i?86-linux (or with -m32 -march=i486). > expand_vector_operations_1 take care about any vector comparison, considering it as a binary operation. See expand_vector_operation and do_compare for more details. > -TARGET_H = $(TM_H) target.h $(TARGET_DEF) insn-modes.h > +TGT = $(TM_H) target.h $(TARGET_DEF) insn-modes.h > > huh, no please ;) I suppose that's no longer necessary anyway now. > Yeah, fixed. :) > I'll leave the i386.c pieces to the x86 target maintainers to review. > They probably will change once the .md file changes are sorted out. If they ever going to be sorted out... > Thanks, > Richard. > Thanks, Artem.
Index: gcc/targhooks.h =================================================================== --- gcc/targhooks.h (revision 177665) +++ gcc/targhooks.h (working copy) @@ -86,6 +86,7 @@ extern int default_builtin_vectorization extern tree default_builtin_reciprocal (unsigned int, bool, bool); extern bool default_builtin_vector_alignment_reachable (const_tree, bool); + extern bool default_builtin_support_vector_misalignment (enum machine_mode mode, const_tree, spurious whitespace change. Index: gcc/optabs.c =================================================================== --- gcc/optabs.c (revision 177665) +++ gcc/optabs.c (working copy) @@ -6572,16 +6572,36 @@ expand_vec_cond_expr (tree vec_cond_type ... + else + { + rtx rtx_op0; + rtx vec; + + rtx_op0 = expand_normal (op0); + comparison = gen_rtx_NE (mode, NULL_RTX, NULL_RTX); + vec = CONST0_RTX (mode); + + create_output_operand (&ops[0], target, mode); + create_input_operand (&ops[1], rtx_op1, mode); + create_input_operand (&ops[2], rtx_op2, mode); + create_input_operand (&ops[3], comparison, mode); + create_input_operand (&ops[4], rtx_op0, mode); + create_input_operand (&ops[5], vec, mode); this still builds the fake(?) != comparison, but as you said you need help with the .md part if we want to use a machine specific pattern for this case (which we eventually want, for the sake of using XOP vcond). Index: gcc/target.h =================================================================== --- gcc/target.h (revision 177665) +++ gcc/target.h (working copy) @@ -51,6 +51,7 @@ #define GCC_TARGET_H #include "insn-modes.h" +#include "gimple.h" #ifdef ENABLE_CHECKING spurious change. @@ -9073,26 +9082,28 @@ fold_comparison (location_t loc, enum tr floating-point, we can only do some of these simplifications.) */