On Mon, Oct 10, 2011 at 12:02 PM, Richard Guenther wrote: > On Fri, Oct 7, 2011 at 9:44 AM, Artem Shinkarov > wrote: >> On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov >> wrote: >>> On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther >>> wrote: >>>> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >>>> wrote: >>>>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>>>> wrote: >>>>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>>>> wrote: >>>>>>> Hi >>>>>>> >>>>>>> Here is a patch to inform a programmer about the expanded vector operation. >>>>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>>>> >>>>>>> ChangeLog: >>>>>>> >>>>>>>        * gcc/tree-vect-generic.c (expand_vector_piecewise): Adjust to >>>>>>>          produce the warning. >>>>>>>          (expand_vector_parallel): Adjust to produce the warning. >>>>>> >>>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog file. >>>>> >>>>> Sure, sorry. >>>>> >>>>>>>          (lower_vec_shuffle): Adjust to produce the warning. >>>>>>>        * gcc/common.opt: New warning Wvector-operation-expanded. >>>>>>>        * gcc/doc/invoke.texi: Document the wawning. >>>>>>> >>>>>>> >>>>>>> Ok? >>>>>> >>>>>> I don't like the name -Wvector-operation-expanded.  We emit a >>>>>> similar warning for missed inline expansions with -Winline, so >>>>>> maybe -Wvector-extensions (that's the name that appears >>>>>> in the C extension documentation). >>>>> >>>>> Hm, I don't care much about the name, unless it gets clear what the >>>>> warning is used for.  I am not really sure that Wvector-extensions >>>>> makes it clear.  Also, I don't see anything bad if the warning will >>>>> pop up during the vectorisation. Any vector operation performed >>>>> outside the SIMD accelerator looks suspicious, because it actually >>>>> doesn't improve performance.  Such a warning during the vectorisation >>>>> could mean that a programmer forgot some flag, or the constant >>>>> propagation failed to deliver a constant, or something else. >>>>> >>>>> Conceptually the text I am producing is not really a warning, it is >>>>> more like an information, but I am not aware of the mechanisms that >>>>> would allow me to introduce a flag triggering inform () or something >>>>> similar. >>>>> >>>>> What I think we really need to avoid is including this warning in the >>>>> standard Ox. >>>>> >>>>>> +  location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>> + >>>>>> +  warning_at (loc, OPT_Wvector_operation_expanded, >>>>>> +             "vector operation will be expanded piecewise"); >>>>>> >>>>>>   v = VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta); >>>>>>   for (i = 0; i < nunits; >>>>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>>>>   tree result, compute_type; >>>>>>   enum machine_mode mode; >>>>>>   int n_words = tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_PER_WORD; >>>>>> +  location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>> + >>>>>> +  warning_at (loc, OPT_Wvector_operation_expanded, >>>>>> +             "vector operation will be expanded in parallel"); >>>>>> >>>>>> what's the difference between 'piecewise' and 'in parallel'? >>>>> >>>>> Parallel is a little bit better for performance than piecewise. >>>> >>>> I see.  That difference should probably be documented, maybe with >>>> an example. >>>> >>>> Richard. >>>> >>>>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>>>>  { >>>>>>   int parts_per_word = UNITS_PER_WORD >>>>>>                       / tree_low_cst (TYPE_SIZE_UNIT (TREE_TYPE (type)), 1); >>>>>> +  location_t loc = gimple_location (gsi_stmt (*gsi)); >>>>>> >>>>>>   if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>>>>       && parts_per_word >= 4 >>>>>>       && TYPE_VECTOR_SUBPARTS (type) >= 4) >>>>>> -    return expand_vector_parallel (gsi, f_parallel, >>>>>> -                                  type, a, b, code); >>>>>> +    return expand_vector_parallel (gsi, f_parallel, type, a, b, code); >>>>>>   else >>>>>> -    return expand_vector_piecewise (gsi, f, >>>>>> -                                   type, TREE_TYPE (type), >>>>>> -                                   a, b, code); >>>>>> +    return expand_vector_piecewise (gsi, f, type, >>>>>> +                                   TREE_TYPE (type), a, b, code); >>>>>>  } >>>>>> >>>>>>  /* Check if vector VEC consists of all the equal elements and >>>>>> >>>>>> unless i miss something loc is unused here.  Please avoid random >>>>>> whitespace changes (just review your patch yourself before posting >>>>>> and revert pieces that do nothing). >>>>> >>>>> Yes you are right, sorry. >>>>> >>>>>> +@item -Wvector-operation-expanded >>>>>> +@opindex Wvector-operation-expanded >>>>>> +@opindex Wno-vector-operation-expanded >>>>>> +Warn if vector operation is not implemented via SIMD capabilities of the >>>>>> +architecture. Mainly useful for the performance tuning. >>>>>> >>>>>> I'd mention that this is for vector operations as of the C extension >>>>>> documented in "Vector Extensions". >>>>>> >>>>>> The vectorizer can produce some operations that will need further >>>>>> lowering - we probably should make sure to _not_ warn about those. >>>>>> Try running the vect.exp testsuite with the new warning turned on >>>>>> (eventually disabling SSE), like with >>>>>> >>>>>> obj/gcc> make check-gcc >>>>>> RUNTESTFLAGS="--target_board=unix/-Wvector-extensions/-mno-sse >>>>>> vect.exp" >>>>> >>>>> Again, see the comment above. I think, if the warning can be triggered >>>>> only manually, then we are fine. But I'll check anyway how many >>>>> warnings I'll get from vect.exp. >>>>> >>>>>>> P.S. It is hard to write a reasonable testcase for the patch, because >>>>>>> one needs to guess which architecture would expand a given vector >>>>>>> operation. But the patch is trivial. >>>>>> >>>>>> You can create an aritificial large vector type for example, or put a >>>>>> testcase under gcc.target/i386 and disable SSE.  We should have >>>>>> a testcase for this. >>>>> >>>>> Yeah, disabling SSE should help. >>>>> >>>>> >>>>> Thanks, >>>>> Artem. >>>>>> Thanks, >>>>>> Richard. >>>>>> >>>>> >>>> >>> >>> New version of the patch in the attachment with the test-cases. >>> Bootstrapped on  x86_64-apple-darwin10.8.0. >>> Currently is being tested. >>> >>> >>> Richard, I've checked the vect.exp case, as you suggested.  It caused >>> a lot of failures, but not because of the new warning.  The main >>> reason is -mno-sse.  The target is capable to vectorize, so the dg >>> option expects tests to pass, but the artificial option makes them >>> fail.  Checking the new warning on vect.exp without -mno-sse, it >>> didn't cause any new failures.  Anyway, we should be pretty much safe, >>> cause the warning is not a part of -O3. >>> >>> Thanks, >>> Artem. >>> >> >> Successfully regression-tested on x86_64-apple-darwin10.8.0. >> >> ChangeLog: >>        gcc/ >>        * doc/invoke.texi: Document new warning. >>        * common.opt (Wvector-operation-performance): Define new warning. >>        * tree-vect-generic.c (expand_vector_piecewise): Warn about expanded >>        vector operation. >>        (exapnd_vector_parallel): Warn about expanded vector operation. >>        (lower_vec_shuffle): Warn about expanded vector operation. >>        * c-parser.c (c_parser_postfix_expression): Assign correct location >>        when creating VEC_SHUFFLE_EXPR. >> >>        gcc/testsuite/ >>        * gcc.target/i386/warn-vect-op-3.c: New test. >>        * gcc.target/i386/warn-vect-op-1.c: New test. >>        * gcc.target/i386/warn-vect-op-2.c: New test. >> >> Ok for trunk? > > +  if (gimple_expr_type (gsi_stmt (*gsi)) == type) > +    warning_at (loc, OPT_Wvector_operation_performance, > +               "vector operation will be expanded piecewise"); > +  else > +    warning_at (loc, OPT_Wvector_operation_performance, > +               "vector operation will be expanded in parallel"); > > we should not check for exact type equivalence here.  Please > use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type) > instead.  We could also consider to pass down the kind of lowering > from the caller (or warn in the callers). Ok, Fixed. > > @@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter >       mode = mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT, 0); >       compute_type = lang_hooks.types.type_for_mode (mode, 1); >       result = f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE, code); > +      warning_at (loc, OPT_Wvector_operation_performance, > +                 "vector operation will be expanded with a " > +                 "single scalar operation"); > > That means it will be fast, no?  Why warn for it at all? Most likely it means sower. Eventually it is a different kind of the expansion. This situation could happen when you work with MMX vectors, and by some reason instead of a single MMX operation, you will have register operation + masking. > > @@ -308,7 +320,7 @@ expand_vector_addition (gimple_stmt_iter >     return expand_vector_parallel (gsi, f_parallel, >                                   type, a, b, code); >   else > -    return expand_vector_piecewise (gsi, f, > +    return expand_vector_piecewise (gsi, f, >                                    type, TREE_TYPE (type), >                                    a, b, code); >  } > > You add trailing space here ... (please review your patches yourself > for this kind of errors) > > +             { > +               expr.value = > +                 c_build_vec_shuffle_expr > +                   (loc, VEC_index (c_expr_t, cexpr_list, 0)->value, > +                    NULL_TREE, VEC_index (c_expr_t, cexpr_list, 1)->value); > +               SET_EXPR_LOCATION (expr.value, loc); > > That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_expr. > If then that routine needs fixing. Ok, moved to c-typeck.c. The new version is in the attachment. Boostrapped on x86_64-apple-darwin10.8.0. Ok? Thanks, Artem. > Thanks, > Richard. > >> >> Thanks, >> Artem. >> >