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~