From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 22459 invoked by alias); 10 Oct 2011 11:02:43 -0000 Received: (qmail 22451 invoked by uid 22791); 10 Oct 2011 11:02:41 -0000 X-SWARE-Spam-Status: No, hits=-2.3 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,FREEMAIL_FROM,RCVD_IN_DNSWL_LOW X-Spam-Check-By: sourceware.org Received: from mail-yx0-f175.google.com (HELO mail-yx0-f175.google.com) (209.85.213.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 10 Oct 2011 11:02:25 +0000 Received: by yxj17 with SMTP id 17so6176324yxj.20 for ; Mon, 10 Oct 2011 04:02:25 -0700 (PDT) MIME-Version: 1.0 Received: by 10.150.188.9 with SMTP id l9mr5907119ybf.15.1318244545003; Mon, 10 Oct 2011 04:02:25 -0700 (PDT) Received: by 10.151.9.9 with HTTP; Mon, 10 Oct 2011 04:02:24 -0700 (PDT) In-Reply-To: References: Date: Mon, 10 Oct 2011 11:15:00 -0000 Message-ID: Subject: Re: New warning for expanded vector operations From: Richard Guenther To: Artem Shinkarov Cc: GCC Patches Content-Type: text/plain; charset=ISO-8859-1 Content-Transfer-Encoding: quoted-printable X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2011-10/txt/msg00739.txt.bz2 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 ope= ration. >>>>>> Bootstrapped on x86-unknown-linux-gnu. >>>>>> >>>>>> ChangeLog: >>>>>> >>>>>> =A0 =A0 =A0 =A0* gcc/tree-vect-generic.c (expand_vector_piecewise): = Adjust to >>>>>> =A0 =A0 =A0 =A0 =A0produce the warning. >>>>>> =A0 =A0 =A0 =A0 =A0(expand_vector_parallel): Adjust to produce the w= arning. >>>>> >>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog fi= le. >>>> >>>> Sure, sorry. >>>> >>>>>> =A0 =A0 =A0 =A0 =A0(lower_vec_shuffle): Adjust to produce the warnin= g. >>>>>> =A0 =A0 =A0 =A0* gcc/common.opt: New warning Wvector-operation-expan= ded. >>>>>> =A0 =A0 =A0 =A0* gcc/doc/invoke.texi: Document the wawning. >>>>>> >>>>>> >>>>>> Ok? >>>>> >>>>> I don't like the name -Wvector-operation-expanded. =A0We 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. =A0I am not really sure that Wvector-extensions >>>> makes it clear. =A0Also, 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. =A0Such 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. >>>> >>>>> + =A0location_t loc =3D gimple_location (gsi_stmt (*gsi)); >>>>> + >>>>> + =A0warning_at (loc, OPT_Wvector_operation_expanded, >>>>> + =A0 =A0 =A0 =A0 =A0 =A0 "vector operation will be expanded piecewis= e"); >>>>> >>>>> =A0 v =3D VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / delta= ); >>>>> =A0 for (i =3D 0; i < nunits; >>>>> @@ -260,6 +264,10 @@ expand_vector_parallel (gimple_stmt_iter >>>>> =A0 tree result, compute_type; >>>>> =A0 enum machine_mode mode; >>>>> =A0 int n_words =3D tree_low_cst (TYPE_SIZE_UNIT (type), 1) / UNITS_P= ER_WORD; >>>>> + =A0location_t loc =3D gimple_location (gsi_stmt (*gsi)); >>>>> + >>>>> + =A0warning_at (loc, OPT_Wvector_operation_expanded, >>>>> + =A0 =A0 =A0 =A0 =A0 =A0 "vector operation will be expanded in paral= lel"); >>>>> >>>>> what's the difference between 'piecewise' and 'in parallel'? >>>> >>>> Parallel is a little bit better for performance than piecewise. >>> >>> I see. =A0That difference should probably be documented, maybe with >>> an example. >>> >>> Richard. >>> >>>>> @@ -301,16 +309,15 @@ expand_vector_addition (gimple_stmt_iter >>>>> =A0{ >>>>> =A0 int parts_per_word =3D UNITS_PER_WORD >>>>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 / tree_low_cst (TYPE_SIZE= _UNIT (TREE_TYPE (type)), 1); >>>>> + =A0location_t loc =3D gimple_location (gsi_stmt (*gsi)); >>>>> >>>>> =A0 if (INTEGRAL_TYPE_P (TREE_TYPE (type)) >>>>> =A0 =A0 =A0 && parts_per_word >=3D 4 >>>>> =A0 =A0 =A0 && TYPE_VECTOR_SUBPARTS (type) >=3D 4) >>>>> - =A0 =A0return expand_vector_parallel (gsi, f_parallel, >>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= type, a, b, code); >>>>> + =A0 =A0return expand_vector_parallel (gsi, f_parallel, type, a, b, = code); >>>>> =A0 else >>>>> - =A0 =A0return expand_vector_piecewise (gsi, f, >>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= type, TREE_TYPE (type), >>>>> - =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= a, b, code); >>>>> + =A0 =A0return expand_vector_piecewise (gsi, f, type, >>>>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0= TREE_TYPE (type), a, b, code); >>>>> =A0} >>>>> >>>>> =A0/* Check if vector VEC consists of all the equal elements and >>>>> >>>>> unless i miss something loc is unused here. =A0Please 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=3D"--target_board=3Dunix/-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. =A0We 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 =A0x86_64-apple-darwin10.8.0. >> Currently is being tested. >> >> >> Richard, I've checked the vect.exp case, as you suggested. =A0It caused >> a lot of failures, but not because of the new warning. =A0The main >> reason is -mno-sse. =A0The target is capable to vectorize, so the dg >> option expects tests to pass, but the artificial option makes them >> fail. =A0Checking the new warning on vect.exp without -mno-sse, it >> didn't cause any new failures. =A0Anyway, 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: > =A0 =A0 =A0 =A0gcc/ > =A0 =A0 =A0 =A0* doc/invoke.texi: Document new warning. > =A0 =A0 =A0 =A0* common.opt (Wvector-operation-performance): Define new w= arning. > =A0 =A0 =A0 =A0* tree-vect-generic.c (expand_vector_piecewise): Warn abou= t expanded > =A0 =A0 =A0 =A0vector operation. > =A0 =A0 =A0 =A0(exapnd_vector_parallel): Warn about expanded vector opera= tion. > =A0 =A0 =A0 =A0(lower_vec_shuffle): Warn about expanded vector operation. > =A0 =A0 =A0 =A0* c-parser.c (c_parser_postfix_expression): Assign correct= location > =A0 =A0 =A0 =A0when creating VEC_SHUFFLE_EXPR. > > =A0 =A0 =A0 =A0gcc/testsuite/ > =A0 =A0 =A0 =A0* gcc.target/i386/warn-vect-op-3.c: New test. > =A0 =A0 =A0 =A0* gcc.target/i386/warn-vect-op-1.c: New test. > =A0 =A0 =A0 =A0* gcc.target/i386/warn-vect-op-2.c: New test. > > Ok for trunk? + if (gimple_expr_type (gsi_stmt (*gsi)) =3D=3D 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). @@ -284,6 +293,9 @@ expand_vector_parallel (gimple_stmt_iter mode =3D mode_for_size (tree_low_cst (TYPE_SIZE (type), 1), MODE_INT= , 0); compute_type =3D lang_hooks.types.type_for_mode (mode, 1); result =3D 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? @@ -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 =3D + 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. Thanks, Richard. > > Thanks, > Artem. >