From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 1533 invoked by alias); 5 Oct 2011 11:35:52 -0000 Received: (qmail 1383 invoked by uid 22791); 5 Oct 2011 11:35:51 -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; Wed, 05 Oct 2011 11:35:36 +0000 Received: by yxj17 with SMTP id 17so1658118yxj.20 for ; Wed, 05 Oct 2011 04:35:35 -0700 (PDT) MIME-Version: 1.0 Received: by 10.150.176.13 with SMTP id y13mr2068986ybe.67.1317814535351; Wed, 05 Oct 2011 04:35:35 -0700 (PDT) Received: by 10.151.9.9 with HTTP; Wed, 5 Oct 2011 04:35:35 -0700 (PDT) In-Reply-To: References: Date: Wed, 05 Oct 2011 11:37: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/msg00321.txt.bz2 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 operat= ion. >>> Bootstrapped on x86-unknown-linux-gnu. >>> >>> ChangeLog: >>> >>> =A0 =A0 =A0 =A0* gcc/tree-vect-generic.c (expand_vector_piecewise): Adj= ust to >>> =A0 =A0 =A0 =A0 =A0produce the warning. >>> =A0 =A0 =A0 =A0 =A0(expand_vector_parallel): Adjust to produce the warn= ing. >> >> 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 warning. >>> =A0 =A0 =A0 =A0* gcc/common.opt: New warning Wvector-operation-expanded. >>> =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 piecewise"); >> >> =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_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 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 >> =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_UN= IT (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 =A0typ= e, a, b, code); >> + =A0 =A0return expand_vector_parallel (gsi, f_parallel, type, a, b, cod= e); >> =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 ty= pe, 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 TR= EE_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. >> >