From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 23308 invoked by alias); 11 Oct 2011 16:12:41 -0000 Received: (qmail 23272 invoked by uid 22791); 11 Oct 2011 16:12:38 -0000 X-SWARE-Spam-Status: No, hits=-2.1 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-vx0-f175.google.com (HELO mail-vx0-f175.google.com) (209.85.220.175) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Tue, 11 Oct 2011 16:12:18 +0000 Received: by vcbf1 with SMTP id f1so363355vcb.20 for ; Tue, 11 Oct 2011 09:12:17 -0700 (PDT) Received: by 10.52.95.164 with SMTP id dl4mr19088062vdb.72.1318349537076; Tue, 11 Oct 2011 09:12:17 -0700 (PDT) MIME-Version: 1.0 Received: by 10.220.150.13 with HTTP; Tue, 11 Oct 2011 09:11:57 -0700 (PDT) In-Reply-To: References: From: Artem Shinkarov Date: Tue, 11 Oct 2011 17:26:00 -0000 Message-ID: Subject: Re: New warning for expanded vector operations To: Richard Guenther 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/msg00906.txt.bz2 On Tue, Oct 11, 2011 at 11:52 AM, Richard Guenther wrote: > On Mon, Oct 10, 2011 at 3:21 PM, Artem Shinkarov > wrote: >> 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: >>>>>>>>> >>>>>>>>> =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 th= e warning. >>>>>>>> >>>>>>>> Entries start without gcc/, they are relative to the gcc/ChangeLog= file. >>>>>>> >>>>>>> Sure, sorry. >>>>>>> >>>>>>>>> =A0 =A0 =A0 =A0 =A0(lower_vec_shuffle): Adjust to produce the war= ning. >>>>>>>>> =A0 =A0 =A0 =A0* gcc/common.opt: New warning Wvector-operation-ex= panded. >>>>>>>>> =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 wi= ll >>>>>>> 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 vectorisa= tion >>>>>>> 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 t= he >>>>>>> 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 piece= wise"); >>>>>>>> >>>>>>>> =A0 v =3D VEC_alloc(constructor_elt, gc, (nunits + delta - 1) / de= lta); >>>>>>>> =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) / UNIT= S_PER_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 pa= rallel"); >>>>>>>> >>>>>>>> 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_S= IZE_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 = =A0type, 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 extensi= on >>>>>>>> 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 trigge= red >>>>>>> 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, bec= ause >>>>>>>>> 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 pu= t 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 caus= ed >>>>> 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 sa= fe, >>>>> 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 ne= w warning. >>>> =A0 =A0 =A0 =A0* tree-vect-generic.c (expand_vector_piecewise): Warn a= bout expanded >>>> =A0 =A0 =A0 =A0vector operation. >>>> =A0 =A0 =A0 =A0(exapnd_vector_parallel): Warn about expanded vector op= eration. >>>> =A0 =A0 =A0 =A0(lower_vec_shuffle): Warn about expanded vector operati= on. >>>> =A0 =A0 =A0 =A0* c-parser.c (c_parser_postfix_expression): Assign corr= ect 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? >>> >>> + =A0if (gimple_expr_type (gsi_stmt (*gsi)) =3D=3D type) >>> + =A0 =A0warning_at (loc, OPT_Wvector_operation_performance, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 "vector operation will be expanded piecew= ise"); >>> + =A0else >>> + =A0 =A0warning_at (loc, OPT_Wvector_operation_performance, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 "vector operation will be expanded in par= allel"); >>> >>> we should not check for exact type equivalence here. =A0Please >>> use types_compatible_p (gimple_expr_type (gsi_stmt (*gsi)), type) >>> instead. =A0We 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 >>> =A0 =A0 =A0 mode =3D mode_for_size (tree_low_cst (TYPE_SIZE (type), 1),= MODE_INT, 0); >>> =A0 =A0 =A0 compute_type =3D lang_hooks.types.type_for_mode (mode, 1); >>> =A0 =A0 =A0 result =3D f (gsi, compute_type, a, b, NULL_TREE, NULL_TREE= , code); >>> + =A0 =A0 =A0warning_at (loc, OPT_Wvector_operation_performance, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "vector operation will be expanded wi= th a " >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 "single scalar operation"); >>> >>> That means it will be fast, no? =A0Why warn for it at all? >> >> Most likely it means sower. =A0Eventually it is a different kind of the >> expansion. =A0This 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 >>> =A0 =A0 return expand_vector_parallel (gsi, f_parallel, >>> =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 typ= e, a, b, code); >>> =A0 else >>> - =A0 =A0return expand_vector_piecewise (gsi, f, >>> + =A0 =A0return expand_vector_piecewise (gsi, f, >>> =A0 =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 =A0= a, b, code); >>> =A0} >>> >>> You add trailing space here ... (please review your patches yourself >>> for this kind of errors) >>> >>> + =A0 =A0 =A0 =A0 =A0 =A0 { >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 expr.value =3D >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 c_build_vec_shuffle_expr >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 (loc, VEC_index (c_expr_t, cexpr_= list, 0)->value, >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0 =A0NULL_TREE, VEC_index (c_expr_t= , cexpr_list, 1)->value); >>> + =A0 =A0 =A0 =A0 =A0 =A0 =A0 SET_EXPR_LOCATION (expr.value, loc); >>> >>> That looks odd - see the 'loc' argument passed to c_build_vec_shuffle_e= xpr. >>> If then that routine needs fixing. >> >> Ok, moved to c-typeck.c. >> >> >> The new version is in the attachment. =A0Boostrapped on x86_64-apple-dar= win10.8.0. >> Ok? > > Ok with > > @@ -2934,7 +2934,8 @@ c_build_vec_perm_expr (location_t loc, t > > =A0 if (!wrap) > =A0 =A0 ret =3D c_wrap_maybe_const (ret, true); > - > + > + =A0SET_EXPR_LOCATION (ret, loc); > =A0 return ret; > > instead of this use build3_loc (loc, ...) when building the VEC_PERM_EXPR > in the line before this hunk. > > Thanks, > Richard. > >> >> Thanks, >> Artem. >> >> >>> Thanks, >>> Richard. >>> >>>> >>>> Thanks, >>>> Artem. >>>> >>> >> > Committed with the revision 179807. Artem.