On Fri, Oct 7, 2011 at 6:22 AM, Artem Shinkarov <artyom.shinkar...@gmail.com> wrote: > On Wed, Oct 5, 2011 at 12:35 PM, Richard Guenther > <richard.guent...@gmail.com> wrote: >> On Wed, Oct 5, 2011 at 1:28 PM, Artem Shinkarov >> <artyom.shinkar...@gmail.com> wrote: >>> On Wed, Oct 5, 2011 at 9:40 AM, Richard Guenther >>> <richard.guent...@gmail.com> wrote: >>>> On Wed, Oct 5, 2011 at 12:18 AM, Artem Shinkarov >>>> <artyom.shinkar...@gmail.com> 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? Thanks, Artem.