On Wed, Jun 24, 2015 at 11:57 AM, Richard Sandiford <richard.sandif...@arm.com> wrote: > Richard Biener <richard.guent...@gmail.com> writes: >> On Tue, Jun 23, 2015 at 11:27 PM, Marc Glisse <marc.gli...@inria.fr> wrote: >>> On Tue, 23 Jun 2015, Richard Sandiford wrote: >>> >>>> +/* Vector comparisons are defined to produce all-one or all-zero results. >>>> */ >>>> +(simplify >>>> + (vec_cond @0 integer_all_onesp@1 integer_zerop@2) >>>> + (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) >>>> + (convert @0))) >>> >>> >>> I am trying to understand why the test tree_nop_conversion_p is the right >>> one (at least for the transformations not using VIEW_CONVERT_EXPR). By >>> definition of VEC_COND_EXPR, type and TREE_TYPE (@0) are both integer vector >>> types of the same size and number of elements. It thus seems like a >>> conversion is always fine. For vectors, tree_nop_conversion_p apparently >>> only checks that they have the same mode (quite often VOIDmode I guess). >> >> The only conversion we seem to allow is changing the signed vector from >> the comparison result to an unsigned vector (same number of elements >> and same mode of the elements). That is, a check using >> TYPE_MODE (type) == TYPE_MODE (TREE_TYPE (@0)) would probably >> be better (well, technically a TYPE_VECTOR_SUBPARTS && element >> mode compare should be better as generic vectors might not have a vector >> mode). > > OK. The reason I was being paranoid was that I couldn't see anywhere > where we enforced that the vector condition in a VEC_COND had to have > the same element width as the values being selected.
We don't require that indeed. > tree-cfg.c > only checks that rhs2 and rhs3 are compatible with the result. > There doesn't seem to be any checking of rhs1 vs. the other types. > So I wasn't sure whether anything stopped us from, e.g., comparing two > V4HIs and using the result to select between two V4SIs. Nothing does (or should). >> I'm fine with using tree_nop_conversion_p for now. > > I like the suggestion about checking TYPE_VECTOR_SUBPARTS and the element > mode. How about: > > (if (VECTOR_INTEGER_TYPE_P (type) > && TYPE_VECTOR_SUBPARTS (type) == TYPE_VECTOR_SUBPARTS (TREE_TYPE (@0)) > && (TYPE_MODE (TREE_TYPE (type)) > == TYPE_MODE (TREE_TYPE (TREE_TYPE (@0))))) > > (But is it really OK to be adding more mode-based compatibility checks? > I thought you were hoping to move away from modes in the middle end.) The TYPE_MODE check makes the VECTOR_INTEGER_TYPE_P check redundant (the type of a comparison is always a signed vector integer type). Yes, mode-based checks are ok. I don't see us moving away from them. >>>> +/* We could instead convert all instances of the vec_cond to negate, >>>> + but that isn't necessarily a win on its own. */ >> >> so p ? 1 : 0 -> -p? Why isn't that a win on its own? It looks more compact >> at least ;) It would also simplify the patterns below. > > In the past I've dealt with processors where arithmetic wasn't handled > as efficiently as logical ops. Seems like an especial risk for 64-bit > elements, from a quick scan of the i386 scheduling models. But then expansion could undo this ... >> I'm missing a comment on the transform done by the following patterns. > > Heh. The comment was supposed to be describing all four at once. > I originally had then bunched together without whitespace, but it > looked bad. > >>>> +(simplify >>>> + (plus:c @3 (vec_cond @0 integer_each_onep@1 integer_zerop@2)) >>>> + (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) >>>> + (minus @3 (convert @0)))) >>>> + >>>> +(simplify >>>> + (plus:c @3 (view_convert_expr >>> >>> >>> Aren't we suppose to drop _expr in match.pd? >> >> Yes. I probably should adjust genmatch.c to reject the _expr variants ;) > > OK. > >>>> + (vec_cond @0 integer_each_onep@1 integer_zerop@2))) >>>> + (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) >>>> + (minus @3 (convert @0)))) >>>> + >>>> +(simplify >>>> + (minus @3 (vec_cond @0 integer_each_onep@1 integer_zerop@2)) >>>> + (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) >>>> + (plus @3 (convert @0)))) >>>> + >>>> +(simplify >>>> + (minus @3 (view_convert_expr >>>> + (vec_cond @0 integer_each_onep@1 integer_zerop@2))) >>>> + (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) >>>> + (plus @3 (convert @0)))) >>>> + >> >> Generally for sign-conversions of vectors you should use view_convert. > > OK. > >> The above also hints at missing conditional view_convert support >> and a way to iterate over commutative vs. non-commutative ops so >> we could write >> >> (for op (plus:c minus) >> rop (minus plus) >> (op @3 (view_convert? (vec_cond @0 integer_each_onep@1 integer_zerop@2))) >> (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) >> (rop @3 (view_convert @0))))) >> >> I'll see implementing that. > > Looks good. :-) > > I also realised later that: > > /* Vector comparisons are defined to produce all-one or all-zero results. */ > (simplify > (vec_cond @0 integer_all_onesp@1 integer_zerop@2) > (if (tree_nop_conversion_p (type, TREE_TYPE (@0))) > (convert @0))) > > is redundant with some fold-const.c code. If so then you should remove the fold-const.c at the time you add the pattern. Note that ISTR code performing exactly the opposite transform in fold-const.c ... Richard. > Thanks, > Richard >
