| Message ID | CAGYS_T+EYp8FZMDjpW8YRwxH-u1Ut714UpB6BDAikB65H4ZBPQ@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
First, Sergey Ostanevich wrote: > + if (LOOP_VINFO_LOOP (loop_vinfo)->force_vect) > + warning_at (vect_location, OPT_Wopenmp_simd, "vectorization " > + "did not happen for a simd loop"); > + 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). 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"? Additionally, shouldn't that be guarded by "if (warn_openmp_simd &&"? Otherwise the flag status isn't used at all in the whole patch. > +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?) 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". > +@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." 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=. > --- 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.) Tobias
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.
Updated patch with spaces, etc according to check_GNU_style.sh Put guard as per Tobias' request. Is it Ok? On Thu, Nov 21, 2013 at 6:18 PM, Sergey Ostanevich <sergos.gnu@gmail.com> 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.
diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt index 0026683..6173013 100644 --- a/gcc/c-family/c.opt +++ b/gcc/c-family/c.opt @@ -592,6 +592,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 EnabledBy(Wall) +Warn about simd directive is overridden by 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 d5971df..6a40a5d 100644 --- a/gcc/common.opt +++ b/gcc/common.opt @@ -2296,10 +2296,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 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 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 c250385..050bd44 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 @@ -3318,6 +3318,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 @@ -4804,6 +4805,10 @@ attribute. @opindex Woverflow Do not warn about compile-time overflow in constant expressions. +@item -Wopenmp-simd +@opindex Wopenm-simd +Warn if vectorizer cost model overrides simd directive from user. + @item -Woverride-init @r{(C and Objective-C only)} @opindex Woverride-init @opindex Wno-override-init diff --git a/gcc/fortran/lang.opt b/gcc/fortran/lang.opt index 5e09cbd..b43c48c 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 Warning +; 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 83d1f45..977db43 100644 --- a/gcc/tree-vect-data-refs.c +++ b/gcc/tree-vect-data-refs.c @@ -1090,7 +1090,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; } @@ -1200,7 +1201,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; @@ -1429,7 +1430,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 @@ -1437,7 +1438,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 86ebbd2..c11d86d 100644 --- a/gcc/tree-vect-loop.c +++ b/gcc/tree-vect-loop.c @@ -2696,7 +2696,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; @@ -2929,6 +2929,10 @@ vect_estimate_min_profitable_iters (loop_vec_info loop_vinfo, /* vector version will never be profitable. */ else { + if (LOOP_VINFO_LOOP (loop_vinfo)->force_vect) + 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 247bdfd..4b25964 100644 --- a/gcc/tree-vect-slp.c +++ b/gcc/tree-vect-slp.c @@ -2171,7 +2171,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 a6c5b59..56ad92c 100644 --- a/gcc/tree-vectorizer.h +++ b/gcc/tree-vectorizer.h @@ -919,9 +919,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; }
Updated as per Richard and Jakub feedback - assuming the default for simd-cost-model is unlmited by default. Richard - was you Ok with it? 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. /* Source location */ On Wed, Nov 20, 2013 at 6:14 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Wed, Nov 20, 2013 at 02:59:21PM +0100, Richard Biener wrote: >> > --- a/gcc/c-family/c.opt >> > +++ b/gcc/c-family/c.opt >> > @@ -592,6 +592,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(openmp_simd) Warning EnabledBy(Wall) > > Please use Var(warn_openmp_simd) here. > >> > --- a/gcc/common.opt >> > +++ b/gcc/common.opt >> > @@ -2296,6 +2296,10 @@ 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 simd directive >> > + >> > Enum >> > Name(vect_cost_model) Type(enum vect_cost_model) UnknownError(unknown >> > vectorizer cost model %qs) > > I'd say you want to add > EnumValue > Enum(vect_cost_model) String(default) Value(VECT_COST_MODEL_DEFAULT) > here. > >> > @@ -2929,6 +2929,13 @@ vect_estimate_min_profitable_iters >> > (loop_vec_info loop_vinfo, >> > /* vector version will never be profitable. */ >> > else >> > { >> > + if (LOOP_VINFO_LOOP (loop_vinfo)->force_vect) >> > + { >> > + warning_at (LOOP_VINFO_LOC (loop_vinfo), OPT_Wopenmp_simd, >> > + "Vectorization did not happen for " >> > + "the loop labeled as simd."); > > No {} around single stmt then body. Also, diagnostic messages > don't start with a capital letter and don't end with dot. > So > "vectorization did not happen for " > "a simd loop" > or so. > >> > /* 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; >> > + return (flag_vect_cost_model == VECT_COST_MODEL_UNLIMITED >> > + || (loop != NULL >> > + && loop->force_vect >> > + && flag_simd_cost_model == VECT_COST_MODEL_UNLIMITED)); >> > } > > IMNSHO this should instead do: > 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; > so, if user said that -fsimd-cost-model=default, then it should honor > -fvect-cost-model. And, IMHO that should be the default, but I don't > feel strongly about that. > > Jakub