From patchwork Tue Nov 26 09:43:32 2013 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Richard Biener X-Patchwork-Id: 294280 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client did not present a certificate) by ozlabs.org (Postfix) with ESMTPS id F373F2C00A7 for ; Tue, 26 Nov 2013 20:43:52 +1100 (EST) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; q=dns; s=default; b=sK+cImc/Xv8zqktH m7h8fl/2s+brkD+ikUvOtDajmc6n0QeuzBSKGJYvnxojll0RxDqP+V9Sd1VelYKr sqZ+sxidCq8+KwWydiVZJupJkx/ZfpWoFI9HI71AV7aDPH9QA8iKygz4OmXI4uvu zvXxezK3D5l16IMsm9dfvqE2mWw= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender:date :from:to:cc:subject:in-reply-to:message-id:references :mime-version:content-type; s=default; bh=j5mVM0i6vzLcHc7A/csfEx +t0qc=; b=f7S9ij/4XPNvY+9qKv+FWTnVJmr3lHq1L4aZN/1wF6+QEhQWUlbrK1 4OPq/u1HlNl0YQ2Q9NKK63zMokeWYzAR4GGM1niICRy8Ful0XmqMYdrsMizvYob6 F6dAEgZcmuR35Fk9SLSoYLs6nO/FIGGqyuySbzvpbRiFfmbnWYWxs= Received: (qmail 614 invoked by alias); 26 Nov 2013 09:43:43 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 599 invoked by uid 89); 26 Nov 2013 09:43:43 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.3 required=5.0 tests=AWL, BAYES_60, RDNS_NONE, URIBL_BLOCKED autolearn=no version=3.3.2 X-HELO: mx2.suse.de Received: from Unknown (HELO mx2.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Tue, 26 Nov 2013 09:43:41 +0000 Received: from relay2.suse.de (unknown [195.135.220.254]) by mx2.suse.de (Postfix) with ESMTP id 11FF9A7B60; Tue, 26 Nov 2013 10:43:33 +0100 (CET) Date: Tue, 26 Nov 2013 10:43:32 +0100 (CET) From: Richard Biener To: Sergey Ostanevich Cc: Tobias Burnus , Jakub Jelinek , Richard Henderson , Yuri Rumyantsev , gcc-patches , Igor Zamyatin , Areg Melik-Adamyan Subject: Re: [gomp4 simd, RFC] Simple fix to override vectorization cost estimation. In-Reply-To: Message-ID: References: <20131119141127.GT892@tucnak.redhat.com> <20131119144220.GU892@tucnak.redhat.com> <528BC6A1.2040206@net-b.de> <20131120141428.GI892@tucnak.redhat.com> <528D0D79.506@net-b.de> User-Agent: Alpine 2.00 (LNX 1167 2008-08-23) MIME-Version: 1.0 On Mon, 25 Nov 2013, Sergey Ostanevich wrote: > Updated patch with spaces, etc according to check_GNU_style.sh > > Put guard as per Tobias' request. > > Is it Ok? See inline comments below (and Tobias mail). > > > On Thu, Nov 21, 2013 at 6:18 PM, Sergey Ostanevich wrote: > > Tobias, > > > > > >> When I understand the patch correctly, the warning is shown in two cases: > >> a) When the loop could be vectorized but the cost model prevented it > >> b) When the loop couldn't be vectorized because of other reasons (e.g. not > >> vectorizable because of conditional loop exits, incomplete vectorization > >> support by the compiler etc.) > >> > >> Do I correctly understand the warning? I am asking because the *opt and > >> *texi wording suggests that only (a) is the case. - I cannot test as the > >> patch cannot be applied with heavy editing (removal of additional line > >> breaks, taking care of tabs converted into spaces). > > > > I believe it's only for a) case, since warning stays along with the cost > > model report that says only about relative scalar and vector costs of > > iteration. The case of exits and vectorization capabilities is handled earlier, > > since we have some vector code here. > > > > Will try to attach the patch instead of copy-paste here. > > > >> > >> Regarding the warning, I think it sounds a bit colloquial and as if the > >> location information is not available. What do you think of "loop with simd > >> directive not vectorized" or concise not fully correct: "simd loop not > >> vectorized"? > > > > took one of yours. > > > >> > >> Additionally, shouldn't that be guarded by "if (warn_openmp_simd &&"? > >> Otherwise the flag status isn't used at all in the whole patch. > > > > This is strange to me, since it worked as I pass the OPT_Wopenmp_simd > > to the warning_at (). It does: > > show warinig with -Wopenmp-simd > > doesn't show warning with -Wall -Wno-openmp-simd > > > >> > >>> +Wopenmp-simd > >>> +C C++ Var(warn_openmp_simd) Warning EnabledBy(Wall) > >>> +Warn about simd directive is overridden by vectorizer cost model > >> > >> > >> Wording wise, I'd prefer something like: > >> "Warn if an simd directive is overridden by the vectorizer cost model" > >> > >> (Or is it "a simd"? Where are the native speakers when one needs them?) > > > > damn, right! I believe 'a' since simd starts with consonant. > > > >> > >> However, in light of my question above, shouldn't it be "Warn if a loop with > >> simd directive is not vectorized"? > >> > >> > >> > >>> +fsimd-cost-model= > >>> +Common Joined RejectNegative Enum(vect_cost_model) > >>> Var(flag_simd_cost_model) Init(VECT_COST_MODEL_UNLIMITED) > >>> +Specifies the vectorization cost model for code marked with simd > >>> directive > >> > >> > >> I think an article is lacking before "simd". > > > > done. > > > >> > >> > >>> +@item -Wopenmp-simd > >>> +@opindex Wopenm-simd > >>> +Warn if vectorizer cost model overrides simd directive from user. > >> > >> > >> I think that can be expanded a bit. One could also mention OpenMP/Cilk Plus > >> explicitly. Maybe like: "Warn if the vectorizer cost model overrides the > >> OpenMP and Cilk Plus simd directives of the user." > >> > > > > done. > > > >> Or if my reading above is correct, how about something like: "Warn if a loop > >> with OpenMP or Cilk Plus simd directive is not vectorized. If only the cost > >> model prevented the vectorization, the @option{-fsimd-cost-model} option can > >> be used to force the vectorization." > >> > >> Which brings me to my next point: -fvect-cost-model= is not documented. I > >> think some words would be helpful, especially about the valid arguments, the > >> default and how it interacts with -fvect-cost-model=. > > > > done. > > > >> > >> > >>> --- a/gcc/fortran/lang.opt > >>> +++ b/gcc/fortran/lang.opt > >>> > >>> +Wopenmp-simd > >>> +Fortran Warning > >>> +; Documented in C > >> > >> ("Warning" is also not needed as it is taken from c-family/*opt, but it > >> shouldn't harm either.) > > > > done. > > > > Sergos > > > > * common.opt: Added new option -fsimd-cost-model. > > * tree-vectorizer.h (unlimited_cost_model): Interface update > > to rely on particular loop info. > > * tree-vect-data-refs.c (vect_peeling_hash_insert): Update to > > unlimited_cost_model call according to new interface. > > (vect_peeling_hash_choose_best_peeling): Ditto. > > (vect_enhance_data_refs_alignment): Ditto. > > * tree-vect-slp.c: Ditto. > > * tree-vect-loop.c (vect_estimate_min_profitable_iters): Ditto, > > plus issue a warning in case cost model overrides users' directive. > > * c.opt: add openmp-simd warning. > > * lang.opt: Ditto. > > * doc/invoke.texi: Added new openmp-simd warning. 2013-11-25 sergey.y.ostanevich * gcc/c-family/c.opt: Introduced a new openmp-simd warning. No gcc/c-family prefix, c-family has its own ChangeLog. * gcc/fortran/lang.opt: Ditto. Likewise * gcc/common.opt: Introduced a new option -fsimd-cost-model. No gcc/ prefix * gcc/doc/invoke.texi: Introduced a new openmp-simd warning and a new -fsimd-cost-model option. * gcc/tree-vectorizer.h (unlimited_cost_model): Interface updated to rely on the particular loop info. * gcc/tree-vect-data-refs.c (vect_peeling_hash_insert): Ditto. (vect_peeling_hash_choose_best_peeling): Ditto. (vect_enhance_data_refs_alignment): Ditto. * gcc/tree-vect-slp.c (vect_slp_analyze_bb_1): Ditto. * gcc/tree-vect-loop.c (vect_estimate_min_profitable_iters): Ditto plus added openmp-simd warining. diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index ac67885..2e9a3df 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -596,6 +596,10 @@ Wold-style-definition C ObjC Var(warn_old_style_definition) Warning Warn if an old-style parameter definition is used +Wopenmp-simd +C C++ Var(warn_openmp_simd) Warning LangEnabledBy(C C++,Wall) +Warn if a simd directive is overridden by the vectorizer cost model + Woverlength-strings C ObjC C++ ObjC++ Var(warn_overlength_strings) Warning LangEnabledBy(C ObjC C++ ObjC++,Wpedantic) Warn if a string is longer than the maximum portable length specified by the standard diff --git a/gcc/common.opt b/gcc/common.opt index a7af636..a7f4f2f 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -2300,10 +2300,17 @@ fvect-cost-model= Common Joined RejectNegative Enum(vect_cost_model) Var(flag_vect_cost_model) Init(VECT_COST_MODEL_DEFAULT) Specifies the cost model for vectorization +fsimd-cost-model= +Common Joined RejectNegative Enum(vect_cost_model) Var(flag_simd_cost_model) Init(VECT_COST_MODEL_UNLIMITED) +Specifies the vectorization cost model for code marked with a simd directive + Enum Name(vect_cost_model) Type(enum vect_cost_model) UnknownError(unknown vectorizer cost model %qs) EnumValue +Enum(vect_cost_model) String(default) Value(VECT_COST_MODEL_DEFAULT) + +EnumValue Don't add "default", it should be the default ;) (so yes, please change the default to VECT_COST_MODEL_DEFAULT) Enum(vect_cost_model) String(unlimited) Value(VECT_COST_MODEL_UNLIMITED) EnumValue diff --git a/gcc/doc/invoke.texi b/gcc/doc/invoke.texi index 501d080..5c8f08f 100644 --- a/gcc/doc/invoke.texi +++ b/gcc/doc/invoke.texi @@ -256,7 +256,7 @@ Objective-C and Objective-C++ Dialects}. -Wlogical-op -Wlong-long @gol -Wmain -Wmaybe-uninitialized -Wmissing-braces -Wmissing-field-initializers @gol -Wmissing-include-dirs @gol --Wno-multichar -Wnonnull -Wno-overflow @gol +-Wno-multichar -Wnonnull -Wno-overflow -Wopenmp-simd @gol -Woverlength-strings -Wpacked -Wpacked-bitfield-compat -Wpadded @gol -Wparentheses -Wpedantic-ms-format -Wno-pedantic-ms-format @gol -Wpointer-arith -Wno-pointer-to-int-cast @gol @@ -3321,6 +3321,7 @@ Options} and @ref{Objective-C and Objective-C++ Dialect Options}. -Wmaybe-uninitialized @gol -Wmissing-braces @r{(only for C/ObjC)} @gol -Wnonnull @gol +-Wopenmp-simd @gol -Wparentheses @gol -Wpointer-sign @gol -Wreorder @gol @@ -4815,6 +4816,12 @@ attribute. @opindex Woverflow Do not warn about compile-time overflow in constant expressions. +@item -Wopenmp-simd +@opindex Wopenm-simd +Warn if the vectorizer cost model overrides the OpenMP or the Cilk Plus +simd directive set by user. The @option{-fsimd-cost-model=unlimited} can +be used to relax the cost model. + @item -Woverride-init @r{(C and Objective-C only)} @opindex Woverride-init @opindex Wno-override-init @@ -8071,6 +8078,15 @@ is equal to the @code{dynamic} model. The default cost model depends on other optimization flags and is either @code{dynamic} or @code{cheap}. +@item -fsimd-cost-model=@var{model} +@opindex fsimd-cost-model +Alter the cost model used for vectorization of loops marked with the OpenMP +or Cilk Plus simd directive. The @var{model} argument should be one of +@code{unlimited}, @code{dynamic}, @code{cheap} or @code{default}. The +@code{default} model means to reuse model defined by @option{fvect-cost-model}. +All other values of @var{model} have the same meaning as described in +@option{fvect-cost-model}. + @item -ftree-vrp @opindex ftree-vrp Perform Value Range Propagation on trees. This is similar to the diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt index 5e09cbd..0d328c8 100644 --- a/gcc/fortran/lang.opt +++ b/gcc/fortran/lang.opt @@ -257,6 +257,10 @@ Wintrinsics-std Fortran Warning Warn on intrinsics not part of the selected standard +Wopenmp-simd +Fortran +; Documented in C + Wreal-q-constant Fortran Warning Warn about real-literal-constants with 'q' exponent-letter diff --git a/gcc/tree-vect-data-refs.c b/gcc/tree-vect-data-refs.c index 5e3b520..dd0ecc4 100644 --- a/gcc/tree-vect-data-refs.c +++ b/gcc/tree-vect-data-refs.c @@ -1096,7 +1096,8 @@ vect_peeling_hash_insert (loop_vec_info loop_vinfo, struct data_reference *dr, *new_slot = slot; } - if (!supportable_dr_alignment && unlimited_cost_model ()) + if (!supportable_dr_alignment + && unlimited_cost_model (LOOP_VINFO_LOOP (loop_vinfo))) slot->count += VECT_MAX_COST; } @@ -1206,7 +1207,7 @@ vect_peeling_hash_choose_best_peeling (loop_vec_info loop_vinfo, res.peel_info.dr = NULL; res.body_cost_vec = stmt_vector_for_cost (); - if (!unlimited_cost_model ()) + if (!unlimited_cost_model (LOOP_VINFO_LOOP (loop_vinfo))) { res.inside_cost = INT_MAX; res.outside_cost = INT_MAX; @@ -1435,7 +1436,7 @@ vect_enhance_data_refs_alignment (loop_vec_info loop_vinfo) vectorization factor. We do this automtically for cost model, since we calculate cost for every peeling option. */ - if (unlimited_cost_model ()) + if (unlimited_cost_model (LOOP_VINFO_LOOP (loop_vinfo))) possible_npeel_number = vf /nelements; /* Handle the aligned case. We may decide to align some other @@ -1443,7 +1444,7 @@ vect_enhance_data_refs_alignment (loop_vec_info loop_vinfo) if (DR_MISALIGNMENT (dr) == 0) { npeel_tmp = 0; - if (unlimited_cost_model ()) + if (unlimited_cost_model (LOOP_VINFO_LOOP (loop_vinfo))) possible_npeel_number++; } diff --git a/gcc/tree-vect-loop.c b/gcc/tree-vect-loop.c index c91c2e1..fc8fa95 100644 --- a/gcc/tree-vect-loop.c +++ b/gcc/tree-vect-loop.c @@ -2703,7 +2703,7 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo, void *target_cost_data = LOOP_VINFO_TARGET_COST_DATA (loop_vinfo); /* Cost model disabled. */ - if (unlimited_cost_model ()) + if (unlimited_cost_model (LOOP_VINFO_LOOP (loop_vinfo))) { dump_printf_loc (MSG_NOTE, vect_location, "cost model disabled.\n"); *ret_min_profitable_niters = 0; @@ -2936,6 +2936,10 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo, /* vector version will never be profitable. */ else { + if (warn_openmp_simd && LOOP_VINFO_LOOP (loop_vinfo)->force_vect) Testing warn_openmp_simd is redundant here, it is taken care of by warning_at and the OPT_Wopenmp_simd flag you passed Otherwise the patch looks ok to me and is good for trunk. Thanks, Richard. + warning_at (vect_location, OPT_Wopenmp_simd, "vectorization " + "did not happen for a simd loop"); + if (dump_enabled_p ()) dump_printf_loc (MSG_MISSED_OPTIMIZATION, vect_location, "cost model: the vector iteration cost = %d " diff --git a/gcc/tree-vect-slp.c b/gcc/tree-vect-slp.c index 680a6d8..2387c0d 100644 --- a/gcc/tree-vect-slp.c +++ b/gcc/tree-vect-slp.c @@ -2176,7 +2176,7 @@ vect_slp_analyze_bb_1 (basic_block bb) } /* Cost model: check if the vectorization is worthwhile. */ - if (!unlimited_cost_model () + if (!unlimited_cost_model (NULL) && !vect_bb_vectorization_profitable_p (bb_vinfo)) { if (dump_enabled_p ()) diff --git a/gcc/tree-vectorizer.h b/gcc/tree-vectorizer.h index 58884f8..8013983 100644 --- a/gcc/tree-vectorizer.h +++ b/gcc/tree-vectorizer.h @@ -910,9 +910,12 @@ known_alignment_for_access_p (struct data_reference *data_ref_info) /* Return true if the vect cost model is unlimited. */ static inline bool -unlimited_cost_model () +unlimited_cost_model (loop_p loop) { - return flag_vect_cost_model == VECT_COST_MODEL_UNLIMITED; + if (loop != NULL && loop->force_vect + && flag_simd_cost_model != VECT_COST_MODEL_DEFAULT) + return flag_simd_cost_model == VECT_COST_MODEL_UNLIMITED; + return (flag_vect_cost_model == VECT_COST_MODEL_UNLIMITED); } /* Source location */