diff mbox

[gomp4,simd,RFC] Simple fix to override vectorization cost estimation.

Message ID alpine.LNX.2.00.1311261037130.8615@zhemvz.fhfr.qr
State New
Headers show

Commit Message

Richard Biener Nov. 26, 2013, 9:43 a.m. UTC
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 <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.

2013-11-25  sergey.y.ostanevich  <sergos.gnu@gmail.com>

	* 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.

Comments

Jakub Jelinek Nov. 26, 2013, 9:46 a.m. UTC | #1
On Tue, Nov 26, 2013 at 10:43:32AM +0100, Richard Biener wrote:
> 2013-11-25  sergey.y.ostanevich  <sergos.gnu@gmail.com>

Please use your name with capital letters and spaces rather than
all lowercase plus dots.

	Jakub
diff mbox

Patch

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 */