2015-09-17 20:35 GMT+03:00 Richard Henderson <r...@redhat.com>:
> 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.
>
Great!
>
>> +/* 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...
Every target would have nunits = vector_size / elem_size because
nunits is used to create a vector mode. Thus no difference.
>
>> +#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?
OK
>
>> @@ -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?
Didn't think about that. I'll give it a try.
>
> I assume you tested all this code with -mno-sse or equivalent arch default?
I didn't make some special runs for that. Just used regression testing
which seems to have such tests.
>
>> @@ -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?
Mask mode was implicit for masked loads and stores. Now it becomes
explicit because we may load the same value using different masks.
E.g. for i386 we may load 256bit vector using both vector and scalar
masks.
>
>> + 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...
Currently we just shouldn't generate such vec_cond. But later I want
to enable such statements generation and this will need to handle non
vector masks. I don't have it in my patches set yet. But it is not
finished and have lots of unfinished work. I don't expect detailed
review for these patches. Once we make a decision that this
representation works fine and we want to have, I'll address opened
issues and send it a separate series.
Thanks,
Ilya
>
>
> r~
