On 09/15/2015 06:52 AM, Ilya Enkovich wrote:
> I made a step forward forcing vector comparisons have a mask (vec<bool>)
> result and disabling bool patterns in case vector comparison is supported by
> target. Several issues were met.
>
> - c/c++ front-ends generate vector comparison with integer vector result. I
> had to make some modifications to use vec_cond instead. Don't know if there
> are other front-ends producing vector comparisons.
> - vector lowering fails to expand vector masks due to mismatch of type and
> mode sizes. I fixed vector type size computation to match mode size and
> added a special handling of mask expand.
> - I disabled canonical type creation for vector mask because we can't layout
> it with VOID mode. I don't know why we may need a canonical type here. But
> get_mask_mode call may be moved into type layout to get it.
> - Expand of vec<bool> constants/contstructors requires special handling.
> Common case should require target hooks/optabs to expand vector into required
> mode. But I suppose we want to have a generic code to handle vector of int
> mode case to avoid modification of multiple targets which use default
> vec<bool> modes.
>
> Currently 'make check' shows two types of regression.
> - missed vector expression pattern recongnition (MIN, MAX, ABX, VEC_COND).
> This must be due to my front-end changes. Hope it will be easy to fix.
> - missed vectorization. All of them appear due to bool patterns disabling.
> I didn't look into all of them but it seems the main problem is in mixed type
> sizes. With bool patterns and integer vector masks we just put int->(other
> sized int) conversion for masks and it gives us required mask transformation.
> With boolean mask we don't have a proper scalar statements to do that. I
> think mask widening/narrowing may be directly supported in masked statements
> vectorization. Going to look into it.
>
> I attach what I currently have for a prototype. It grows bigger so I split
> into several parts.
The general approach looks good.
> +/* By defaults a vector of integers is used as a mask. */
> +
> +machine_mode
> +default_get_mask_mode (unsigned nunits, unsigned vector_size)
> +{
> + unsigned elem_size = vector_size / nunits;
> + machine_mode elem_mode
> + = smallest_mode_for_size (elem_size * BITS_PER_UNIT, MODE_INT);
Why these arguments as opposed to passing elem_size? It seems that every hook
is going to have to do this division...
> +#define VECTOR_MASK_TYPE_P(TYPE) \
> + (TREE_CODE (TYPE) == VECTOR_TYPE \
> + && TREE_CODE (TREE_TYPE (TYPE)) == BOOLEAN_TYPE)
Perhaps better as VECTOR_BOOLEAN_TYPE_P, since that's exactly what's being
tested?
> @@ -3464,10 +3464,10 @@ verify_gimple_comparison (tree type, tree op0, tree
> op1)
> return true;
> }
> }
> - /* Or an integer vector type with the same size and element count
> + /* Or a boolean vector type with the same element count
> as the comparison operand types. */
> else if (TREE_CODE (type) == VECTOR_TYPE
> - && TREE_CODE (TREE_TYPE (type)) == INTEGER_TYPE)
> + && TREE_CODE (TREE_TYPE (type)) == BOOLEAN_TYPE)
VECTOR_BOOLEAN_TYPE_P.
> @@ -122,7 +122,19 @@ tree_vec_extract (gimple_stmt_iterator *gsi, tree type,
> tree t, tree bitsize, tree bitpos)
> {
> if (bitpos)
> - return gimplify_build3 (gsi, BIT_FIELD_REF, type, t, bitsize, bitpos);
> + {
> + if (TREE_CODE (type) == BOOLEAN_TYPE)
> + {
> + tree itype
> + = build_nonstandard_integer_type (tree_to_uhwi (bitsize), 0);
> + tree field = gimplify_build3 (gsi, BIT_FIELD_REF, itype, t,
> + bitsize, bitpos);
> + return gimplify_build2 (gsi, NE_EXPR, type, field,
> + build_zero_cst (itype));
> + }
> + else
> + return gimplify_build3 (gsi, BIT_FIELD_REF, type, t, bitsize, bitpos);
> + }
> else
> return gimplify_build1 (gsi, VIEW_CONVERT_EXPR, type, t);
> }
So... this is us lowering vector operations on a target that doesn't support
them. Which means that we get to decide what value is produced for a
comparison? Which means that we can settle on the "normal" -1, correct?
Which means that we ought not need to extract the entire element and then
compare for non-zero, but rather simply extract a single bit from the element,
and directly call that a boolean result, correct?
I assume you tested all this code with -mno-sse or equivalent arch default?
> @@ -1885,7 +1885,9 @@ expand_MASK_LOAD (gcall *stmt)
> create_output_operand (&ops[0], target, TYPE_MODE (type));
> create_fixed_operand (&ops[1], mem);
> create_input_operand (&ops[2], mask, TYPE_MODE (TREE_TYPE (maskt)));
> - expand_insn (optab_handler (maskload_optab, TYPE_MODE (type)), 3, ops);
> + expand_insn (convert_optab_handler (maskload_optab, TYPE_MODE (type),
> + TYPE_MODE (TREE_TYPE (maskt))),
> + 3, ops);
Why do we now need a conversion here?
> + if (GET_MODE_CLASS (TYPE_MODE (TREE_TYPE ((op0)))) != MODE_VECTOR_INT)
> + {
> + /* This is a vcond with mask. To be supported soon... */
> + gcc_unreachable ();
> + }
Leave this out til we need it? I can't see that you replace this later in the
patch series...
r~
