On Wed, 6 Apr 2022, Martin Sebor wrote:
> On 4/6/22 03:23, Richard Biener wrote:
> > This avoids -Wvector-operation-performance diagnostics for vectorizer
> > produced code. It's unfortunate the warning_at code in
> > tree-vect-generic.cc needs adjustments but the diagnostic suppression
> > code doesn't magically suppress those otherwise.
>
> It seems like it should, as long as the statement location hasn't
> changed after the suppress_diagnostic call in tree-vect-stmts.cc.
The location doesn't change.
> >
> > Bootstrap / regtest running on x86_64-unknown-linux-gnu.
> >
> > Martin/David - did I miss something obvious when doing the
> > tree-vect-generic.cc adjustment?
>
> The only thing I can think of is that because it's not handled in
> diagnostic-spec.cc, -Wvector-operation-performance is lumped in with
> all other generic warnings that also aren't handled. It means that
> they are all treated as a group. Whether or not that's what we want
> for this specific warning might be something to consider.
So when I call suppress_warning (gimple *, ..) the suppress_warning_at
call constructs a nowarn_spec_t with NW_OTHER, it queries the
nowarn_map where it doesn't find anything yet, and goes on
with
nowarn_map->put (loc, optspec);
return true;
suppress_warning then (since supp is true anyway) goes on with
set_no_warning_bit (stmt, supp);
which is likely what my changes to tree-vect-generic.cc in the end
key on. When I simply invoke
warning_at (loc, OPT_Wvector_operation_performance,
"...")
I see nowhere that nowarn_spec_t::nowarn_spec_t is invoked, nor
is warning_suppressed_at. Maybe I'm missing that being done
but I think that's by design? It at least was surprising to me.
Of course since we lack a warning_at (gimple *, ..) overload
or alternatively extending rich-location to cover diagnostic
suppression contexts, doing this would only work for stmts with
a location that doesn't fall back to that of the current
declaration (for UNKNOWN_LOCATION loc).
So my main question was if the diagnostic suppression is supposed
to be transparently handled by warning_at (...) or whether indeed
explicit guards need to be added to each diagnostic emission.
As I'm now doing
if (!warning_suppressed_p (gsi_stmt (*gsi),
OPT_Wvector_operation_performance))
I get to get_nowarn_spec for the stmt which will return NULL
because the no-warning bit is set (but it's always set in the
warning suppression call when done on a stmt!)
When I'm doing
if (!warning_suppressed_at (loc,
OPT_Wvector_operation_performance))
then it also suppresses the diagnostic but I think I'm not supposed
to call that function since it will ICE on UNKNOWN_LOCATION and it
would lack the fallback to the nowarning bit for stmts without
location.
That is - the stmt-based query looks correct to me but it will
always use the non-specific flag, at least when I suppress the
diagnostic based on the stmt?! I think suppress_warning (gimple *,...)
should not set the no-warning bit when it succeeded in amending
the nowarn_map?
So, again, was the requirement to explicitely guard warning_at ()
calls with warning_suppressed_p () calls by design? If it wasn't
intentional then I think we need to somehow allow to specify
a gimple * or tree as location argument to warning_at, since
we have rich-location it might be tempting to use that as vehicle
to carry the nowarn query result or alternatively the stmt/tree
itself?
Thanks,
Richard.
> Martin
>
> >
> > Thanks,
> > Richard.
> >
> > 2022-04-06 Richard Biener <rguent...@suse.de>
> >
> > PR tree-optimization/105175
> > * tree-vect-stmts.cc (vectorizable_operation): Suppress
> > -Wvector-operation-performance if using emulated vectors.
> > * tree-vect-generic.cc (expand_vector_piecewise): Do not diagnose
> > -Wvector-operation-performance when suppressed.
> > (expand_vector_parallel): Likewise.
> > (expand_vector_comparison): Likewise.
> > (expand_vector_condition): Likewise.
> > (lower_vec_perm): Likewise.
> > (expand_vector_conversion): Likewise.
> >
> > * gcc.dg/pr105175.c: New testcase.
> > ---
> > gcc/testsuite/gcc.dg/pr105175.c | 16 +++++++++++++
> > gcc/tree-vect-generic.cc | 41 ++++++++++++++++++++++-----------
> > gcc/tree-vect-stmts.cc | 2 ++
> > 3 files changed, 45 insertions(+), 14 deletions(-)
> > create mode 100644 gcc/testsuite/gcc.dg/pr105175.c
> >
> > diff --git a/gcc/testsuite/gcc.dg/pr105175.c
> > b/gcc/testsuite/gcc.dg/pr105175.c
> > new file mode 100644
> > index 00000000000..d8d7edb942a
> > --- /dev/null
> > +++ b/gcc/testsuite/gcc.dg/pr105175.c
> > @@ -0,0 +1,16 @@
> > +/* { dg-do compile } */
> > +/* { dg-options "-O2 -Wvector-operation-performance" } */
> > +/* { dg-additional-options "-mno-sse" { target x86_64-*-* i?86-*-* } } */
> > +
> > +enum { QEMU_MIGRATION_COOKIE_PERSISTENT = 1 };
> > +struct {
> > + unsigned flags;
> > + unsigned flagsMandatory;
> > +} qemuMigrationCookieGetPersistent_mig;
> > +void qemuMigrationCookieGetPersistent()
> > +{
> > + qemuMigrationCookieGetPersistent_mig.flags &= /* { dg-bogus "will be
> > expanded" } */
> > + QEMU_MIGRATION_COOKIE_PERSISTENT;
> > + qemuMigrationCookieGetPersistent_mig.flagsMandatory &=
> > + QEMU_MIGRATION_COOKIE_PERSISTENT;
> > +}
> > diff --git a/gcc/tree-vect-generic.cc b/gcc/tree-vect-generic.cc
> > index 12a553ec8be..8b7227e8b58 100644
> > --- a/gcc/tree-vect-generic.cc
> > +++ b/gcc/tree-vect-generic.cc
> > @@ -317,8 +317,11 @@ expand_vector_piecewise (gimple_stmt_iterator *gsi,
> > @@ elem_op_func f,
> > int i;
> > location_t loc = gimple_location (gsi_stmt (*gsi));
> > - if (nunits == 1)
> > - /* Do not diagnose decomposing single element vectors. */
> > + if (nunits == 1
> > + || warning_suppressed_p (gsi_stmt (*gsi),
> > + OPT_Wvector_operation_performance))
> > + /* Do not diagnose decomposing single element vectors or when
> > + decomposing vectorizer produced operations. */
> > ;
> > else if (ret_type || !parallel_p)
> > warning_at (loc, OPT_Wvector_operation_performance,
> > @@ -379,14 +382,16 @@ expand_vector_parallel (gimple_stmt_iterator *gsi,
> > @@ elem_op_func f, tree type,
> > else
> > {
> > /* Use a single scalar operation with a mode no wider than
> > word_mode. */
> > + if (!warning_suppressed_p (gsi_stmt (*gsi),
> > + OPT_Wvector_operation_performance))
> > + warning_at (loc, OPT_Wvector_operation_performance,
> > + "vector operation will be expanded with a "
> > + "single scalar operation");
> > scalar_int_mode mode
> > = int_mode_for_size (tree_to_uhwi (TYPE_SIZE (type)), 0).require ();
> > compute_type = lang_hooks.types.type_for_mode (mode, 1);
> > result = f (gsi, compute_type, a, b, bitsize_zero_node,
> > TYPE_SIZE (compute_type), code, type);
> > - warning_at (loc, OPT_Wvector_operation_performance,
> > - "vector operation will be expanded with a "
> > - "single scalar operation");
> > }
> >
> > return result;
> > @@ -487,8 +492,10 @@ expand_vector_comparison (gimple_stmt_iterator *gsi,
> > @@ tree type, tree op0,
> >
> > if (TYPE_PRECISION (ret_inner_type) != 1)
> > ret_inner_type = build_nonstandard_integer_type (1, 1);
> > - warning_at (loc, OPT_Wvector_operation_performance,
> > - "vector operation will be expanded piecewise");
> > + if (!warning_suppressed_p (gsi_stmt (*gsi),
> > + OPT_Wvector_operation_performance))
> > + warning_at (loc, OPT_Wvector_operation_performance,
> > + "vector operation will be expanded piecewise");
> > for (i = 0; i < nunits;
> > i++, index = int_const_binop (PLUS_EXPR, index, part_width))
> > {
> > @@ -1098,8 +1105,9 @@ expand_vector_condition (gimple_stmt_iterator *gsi,
> > @@ bitmap dce_ssa_names)
> >
> > /* TODO: try and find a smaller vector type. */
> > - warning_at (loc, OPT_Wvector_operation_performance,
> > - "vector condition will be expanded piecewise");
> > + if (!warning_suppressed_p (stmt, OPT_Wvector_operation_performance))
> > + warning_at (loc, OPT_Wvector_operation_performance,
> > + "vector condition will be expanded piecewise");
> >
> > if (!a_is_comparison
> > && VECTOR_BOOLEAN_TYPE_P (TREE_TYPE (a))
> > @@ -1591,9 +1599,10 @@ lower_vec_perm (gimple_stmt_iterator *gsi)
> > }
> > else if (can_vec_perm_var_p (TYPE_MODE (vect_type)))
> > return;
> > -
> > - warning_at (loc, OPT_Wvector_operation_performance,
> > - "vector shuffling operation will be expanded piecewise");
> > +
> > + if (!warning_suppressed_p (stmt, OPT_Wvector_operation_performance))
> > + warning_at (loc, OPT_Wvector_operation_performance,
> > + "vector shuffling operation will be expanded piecewise");
> >
> > vec_alloc (v, elements);
> > bool constant_p = true;
> > @@ -2029,8 +2038,12 @@ expand_vector_conversion (gimple_stmt_iterator *gsi)
> > location_t loc = gimple_location (gsi_stmt (*gsi));
> >
> > if (compute_type != arg_type)
> > - warning_at (loc, OPT_Wvector_operation_performance,
> > - "vector operation will be expanded piecewise");
> > + {
> > + if (!warning_suppressed_p (gsi_stmt (*gsi),
> > +
> > OPT_Wvector_operation_performance))
> > + warning_at (loc, OPT_Wvector_operation_performance,
> > + "vector operation will be expanded
> > piecewise");
> > + }
> > else
> > {
> > nunits = 1;
> > diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> > index 29d6b2a2ae7..3f8d5aac8ee 100644
> > --- a/gcc/tree-vect-stmts.cc
> > +++ b/gcc/tree-vect-stmts.cc
> > @@ -6456,6 +6456,8 @@ vectorizable_operation (vec_info *vinfo,
> > new_temp = make_ssa_name (vec_dest, new_stmt);
> > gimple_assign_set_lhs (new_stmt, new_temp);
> > vect_finish_stmt_generation (vinfo, stmt_info, new_stmt, gsi);
> > + if (using_emulated_vectors_p)
> > + suppress_warning (new_stmt, OPT_Wvector_operation_performance);
> >
> > /* Enter the combined value into the vector cond hash so we don't
> > AND it with a loop mask again. */
>
>
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Ivo Totev; HRB 36809 (AG Nuernberg)
