[RFC] build: enable C++11 narrowing warnings

Tested x86_64-pc-linux-gnu.  OK for trunk?

-- 8< --

We've been using -Wno-narrowing since gcc 4.7, but at this point narrowing
diagnostics seem like a stable part of C++ and we should adjust.

This patch changes -Wno-narrowing to -Wno-error=narrowing so that narrowing
issues will still not break bootstrap, but we can see them.

The rest of the patch fixes the narrowing warnings I see in an
x86_64-pc-linux-gnu bootstrap.  In most of the cases, by adjusting the types
of various declarations so that we store the values in the same types we
compute them in, which seems worthwhile anyway.  This also allowed us to
remove a few -Wsign-compare casts.

The one place I didn't see how best to do this was in
vect_prologue_cost_for_slp: changing const_nunits to unsigned int broke the
call to TYPE_VECTOR_SUBPARTS (vectype).is_constant (&const_nunits), since
poly_uint64.is_constant wants a pointer to unsigned HOST_WIDE_INT.  So I
added casts in that one place.  Not too bad, I think.

gcc/ChangeLog:

	* configure.ac (CXX_WARNING_OPTS): Change -Wno-narrowing
	to -Wno-error=narrowing.
	* configure: Regenerate.
	* config/i386/i386.h (debugger_register_map)
	(debugger64_register_map)
	(svr4_debugger_register_map): Make unsigned.
	* config/i386/i386.cc: Likewise.
	* diagnostic-event-id.h (diagnostic_thread_id_t): Make int.
	* vec.h (vec::size): Make unsigned int.
	* ipa-modref.cc (escape_point::arg): Make unsigned.
	(modref_lattice::add_escape_point): Use eaf_flags_t.
	(update_escape_summary_1): Use eaf_flags_t, && for bool.
	* pair-fusion.cc (pair_fusion_bb_info::track_access):
	Make mem_size unsigned int.
	* pretty-print.cc (format_phase_2): Cast va_arg to char.
	* tree-ssa-loop-ch.cc (ch_base::copy_headers): Make nheaders
	unsigned, remove cast.
	* tree-ssa-structalias.cc (push_fields_onto_fieldstack):
	Make offset unsigned, remove cast.
	* tree-vect-slp.cc (vect_prologue_cost_for_slp): Add cast.
	* tree-vect-stmts.cc (vect_truncate_gather_scatter_offset):
	Make scale unsigned.
	(vectorizable_operation): Make ncopies unsigned.
	* rtl-ssa/member-fns.inl: Make num_accesses unsigned int.
---
 gcc/config/i386/i386.h      |  6 +++---
 gcc/diagnostic-event-id.h   |  2 +-
 gcc/vec.h                   |  2 +-
 gcc/config/i386/i386.cc     |  6 +++---
 gcc/ipa-modref.cc           | 13 +++++++------
 gcc/pair-fusion.cc          |  2 +-
 gcc/pretty-print.cc         |  2 +-
 gcc/tree-ssa-loop-ch.cc     |  6 +++---
 gcc/tree-ssa-structalias.cc |  6 +++---
 gcc/tree-vect-slp.cc        |  3 ++-
 gcc/tree-vect-stmts.cc      |  7 ++++---
 gcc/configure.ac            |  3 +--
 gcc/rtl-ssa/member-fns.inl  |  3 ++-
 gcc/configure               |  7 +++----
 14 files changed, 35 insertions(+), 33 deletions(-)


base-commit: 2484ba167e1c4a52d4989b71e1f5d4d27755500f

On Sat, Sep 21, 2024 at 2:49 AM Jason Merrill <jason@redhat.com> wrote:
>
> Tested x86_64-pc-linux-gnu.  OK for trunk?
>
> -- 8< --
>
> We've been using -Wno-narrowing since gcc 4.7, but at this point narrowing
> diagnostics seem like a stable part of C++ and we should adjust.
>
> This patch changes -Wno-narrowing to -Wno-error=narrowing so that narrowing
> issues will still not break bootstrap, but we can see them.
>
> The rest of the patch fixes the narrowing warnings I see in an
> x86_64-pc-linux-gnu bootstrap.  In most of the cases, by adjusting the types
> of various declarations so that we store the values in the same types we
> compute them in, which seems worthwhile anyway.  This also allowed us to
> remove a few -Wsign-compare casts.
>
> The one place I didn't see how best to do this was in
> vect_prologue_cost_for_slp: changing const_nunits to unsigned int broke the
> call to TYPE_VECTOR_SUBPARTS (vectype).is_constant (&const_nunits), since
> poly_uint64.is_constant wants a pointer to unsigned HOST_WIDE_INT.  So I
> added casts in that one place.  Not too bad, I think.
>
> gcc/ChangeLog:
>
>         * configure.ac (CXX_WARNING_OPTS): Change -Wno-narrowing
>         to -Wno-error=narrowing.
>         * configure: Regenerate.
>         * config/i386/i386.h (debugger_register_map)
>         (debugger64_register_map)
>         (svr4_debugger_register_map): Make unsigned.
>         * config/i386/i386.cc: Likewise.
>         * diagnostic-event-id.h (diagnostic_thread_id_t): Make int.
>         * vec.h (vec::size): Make unsigned int.
>         * ipa-modref.cc (escape_point::arg): Make unsigned.
>         (modref_lattice::add_escape_point): Use eaf_flags_t.
>         (update_escape_summary_1): Use eaf_flags_t, && for bool.
>         * pair-fusion.cc (pair_fusion_bb_info::track_access):
>         Make mem_size unsigned int.
>         * pretty-print.cc (format_phase_2): Cast va_arg to char.
>         * tree-ssa-loop-ch.cc (ch_base::copy_headers): Make nheaders
>         unsigned, remove cast.
>         * tree-ssa-structalias.cc (push_fields_onto_fieldstack):
>         Make offset unsigned, remove cast.
>         * tree-vect-slp.cc (vect_prologue_cost_for_slp): Add cast.
>         * tree-vect-stmts.cc (vect_truncate_gather_scatter_offset):
>         Make scale unsigned.
>         (vectorizable_operation): Make ncopies unsigned.
>         * rtl-ssa/member-fns.inl: Make num_accesses unsigned int.
> ---
>  gcc/config/i386/i386.h      |  6 +++---
>  gcc/diagnostic-event-id.h   |  2 +-
>  gcc/vec.h                   |  2 +-
>  gcc/config/i386/i386.cc     |  6 +++---
>  gcc/ipa-modref.cc           | 13 +++++++------
>  gcc/pair-fusion.cc          |  2 +-
>  gcc/pretty-print.cc         |  2 +-
>  gcc/tree-ssa-loop-ch.cc     |  6 +++---
>  gcc/tree-ssa-structalias.cc |  6 +++---
>  gcc/tree-vect-slp.cc        |  3 ++-
>  gcc/tree-vect-stmts.cc      |  7 ++++---
>  gcc/configure.ac            |  3 +--
>  gcc/rtl-ssa/member-fns.inl  |  3 ++-
>  gcc/configure               |  7 +++----
>  14 files changed, 35 insertions(+), 33 deletions(-)
>
> diff --git a/gcc/config/i386/i386.h b/gcc/config/i386/i386.h
> index c1ec92ffb15..751c250ddb3 100644
> --- a/gcc/config/i386/i386.h
> +++ b/gcc/config/i386/i386.h
> @@ -2091,9 +2091,9 @@ do {                                                      \
>  #define DEBUGGER_REGNO(N) \
>    (TARGET_64BIT ? debugger64_register_map[(N)] : debugger_register_map[(N)])
>
> -extern int const debugger_register_map[FIRST_PSEUDO_REGISTER];
> -extern int const debugger64_register_map[FIRST_PSEUDO_REGISTER];
> -extern int const svr4_debugger_register_map[FIRST_PSEUDO_REGISTER];
> +extern unsigned int const debugger_register_map[FIRST_PSEUDO_REGISTER];
> +extern unsigned int const debugger64_register_map[FIRST_PSEUDO_REGISTER];
> +extern unsigned int const svr4_debugger_register_map[FIRST_PSEUDO_REGISTER];
>
>  /* Before the prologue, RA is at 0(%esp).  */
>  #define INCOMING_RETURN_ADDR_RTX \
> diff --git a/gcc/diagnostic-event-id.h b/gcc/diagnostic-event-id.h
> index 8237ba34df3..06985d23c12 100644
> --- a/gcc/diagnostic-event-id.h
> +++ b/gcc/diagnostic-event-id.h
> @@ -67,6 +67,6 @@ typedef diagnostic_event_id_t *diagnostic_event_id_ptr;
>  /* A type for compactly referring to a particular thread within a
>     diagnostic_path.  Typically there is just one thread per path,
>     with id 0.  */
> -typedef unsigned diagnostic_thread_id_t;
> +typedef int diagnostic_thread_id_t;
>
>  #endif /* ! GCC_DIAGNOSTIC_EVENT_ID_H */
> diff --git a/gcc/vec.h b/gcc/vec.h
> index bc83827f644..b13c4716428 100644
> --- a/gcc/vec.h
> +++ b/gcc/vec.h
> @@ -2409,7 +2409,7 @@ public:
>    const value_type &back () const;
>    const value_type &operator[] (unsigned int i) const;
>
> -  size_t size () const { return m_size; }
> +  unsigned size () const { return m_size; }
>    size_t size_bytes () const { return m_size * sizeof (T); }
>    bool empty () const { return m_size == 0; }
>
> diff --git a/gcc/config/i386/i386.cc b/gcc/config/i386/i386.cc
> index 7dbae1d72e3..2f736a3b346 100644
> --- a/gcc/config/i386/i386.cc
> +++ b/gcc/config/i386/i386.cc
> @@ -181,7 +181,7 @@ enum reg_class const regclass_map[FIRST_PSEUDO_REGISTER] =
>
>  /* The "default" register map used in 32bit mode.  */
>
> -int const debugger_register_map[FIRST_PSEUDO_REGISTER] =
> +unsigned int const debugger_register_map[FIRST_PSEUDO_REGISTER] =
>  {
>    /* general regs */
>    0, 2, 1, 3, 6, 7, 4, 5,
> @@ -212,7 +212,7 @@ int const debugger_register_map[FIRST_PSEUDO_REGISTER] =
>
>  /* The "default" register map used in 64bit mode.  */
>
> -int const debugger64_register_map[FIRST_PSEUDO_REGISTER] =
> +unsigned int const debugger64_register_map[FIRST_PSEUDO_REGISTER] =
>  {
>    /* general regs */
>    0, 1, 2, 3, 4, 5, 6, 7,
> @@ -294,7 +294,7 @@ int const debugger64_register_map[FIRST_PSEUDO_REGISTER] =
>         17 for %st(6) (gcc regno = 14)
>         18 for %st(7) (gcc regno = 15)
>  */
> -int const svr4_debugger_register_map[FIRST_PSEUDO_REGISTER] =
> +unsigned int const svr4_debugger_register_map[FIRST_PSEUDO_REGISTER] =
>  {
>    /* general regs */
>    0, 2, 1, 3, 6, 7, 5, 4,
> diff --git a/gcc/ipa-modref.cc b/gcc/ipa-modref.cc
> index 400a8856de2..19359662f8f 100644
> --- a/gcc/ipa-modref.cc
> +++ b/gcc/ipa-modref.cc
> @@ -1997,7 +1997,7 @@ struct escape_point
>    /* Value escapes to this call.  */
>    gcall *call;
>    /* Argument it escapes to.  */
> -  int arg;
> +  unsigned int arg;
>    /* Flags already known about the argument (this can save us from recording
>       escape points if local analysis did good job already).  */
>    eaf_flags_t min_flags;
> @@ -2047,7 +2047,8 @@ public:
>    bool merge_deref (const modref_lattice &with, bool ignore_stores);
>    bool merge_direct_load ();
>    bool merge_direct_store ();
> -  bool add_escape_point (gcall *call, int arg, int min_flags, bool diret);
> +  bool add_escape_point (gcall *call, unsigned int arg,
> +                        eaf_flags_t min_flags, bool direct);
>    void dump (FILE *out, int indent = 0) const;
>  };
>
> @@ -2101,8 +2102,8 @@ modref_lattice::dump (FILE *out, int indent) const
>     point exists.  */
>
>  bool
> -modref_lattice::add_escape_point (gcall *call, int arg, int min_flags,
> -                                 bool direct)
> +modref_lattice::add_escape_point (gcall *call, unsigned arg,
> +                                 eaf_flags_t min_flags, bool direct)
>  {
>    escape_point *ep;
>    unsigned int i;
> @@ -4415,12 +4416,12 @@ update_escape_summary_1 (cgraph_edge *e,
>         continue;
>        FOR_EACH_VEC_ELT (map[ee->parm_index], j, em)
>         {
> -         int min_flags = ee->min_flags;
> +         eaf_flags_t min_flags = ee->min_flags;
>           if (ee->direct && !em->direct)
>             min_flags = deref_flags (min_flags, ignore_stores);
>           struct escape_entry entry = {em->parm_index, ee->arg,
>                                        min_flags,
> -                                      ee->direct & em->direct};
> +                                      ee->direct && em->direct};
>           sum->esc.safe_push (entry);
>         }
>      }
> diff --git a/gcc/pair-fusion.cc b/gcc/pair-fusion.cc
> index cb0374f426b..653055fdcf6 100644
> --- a/gcc/pair-fusion.cc
> +++ b/gcc/pair-fusion.cc
> @@ -444,7 +444,7 @@ pair_fusion_bb_info::track_access (insn_info *insn, bool load_p, rtx mem)
>    const bool fpsimd_op_p = m_pass->fpsimd_op_p (reg_op, mem_mode, load_p);
>
>    // Note pair_operand_mode_ok_p already rejected VL modes.
> -  const HOST_WIDE_INT mem_size = GET_MODE_SIZE (mem_mode).to_constant ();
> +  const unsigned mem_size = GET_MODE_SIZE (mem_mode).to_constant ();
>    const lfs_fields lfs = { load_p, fpsimd_op_p, mem_size };
>
>    if (track_via_mem_expr (insn, mem, lfs))
> diff --git a/gcc/pretty-print.cc b/gcc/pretty-print.cc
> index 998e06e155f..0cd9b4d0a41 100644
> --- a/gcc/pretty-print.cc
> +++ b/gcc/pretty-print.cc
> @@ -1923,7 +1923,7 @@ format_phase_2 (pretty_printer *pp,
>             /* When quoting, print alphanumeric, punctuation, and the space
>                character unchanged, and all others in hexadecimal with the
>                "\x" prefix.  Otherwise print them all unchanged.  */
> -           int chr = va_arg (*text.m_args_ptr, int);
> +           char chr = (char) va_arg (*text.m_args_ptr, int);
>             if (ISPRINT (chr) || !quote)
>               pp_character (pp, chr);
>             else
> diff --git a/gcc/tree-ssa-loop-ch.cc b/gcc/tree-ssa-loop-ch.cc
> index 525eb357858..6552ddd1ee2 100644
> --- a/gcc/tree-ssa-loop-ch.cc
> +++ b/gcc/tree-ssa-loop-ch.cc
> @@ -839,8 +839,8 @@ ch_base::copy_headers (function *fun)
>          copied.  TODO -- handle while (a || b) - like cases, by not requiring
>          the header to have just a single successor and copying up to
>          postdominator.  */
> -      int nheaders = 0;
> -      int last_win_nheaders = 0;
> +      unsigned int nheaders = 0;
> +      unsigned int last_win_nheaders = 0;
>        bool last_win_invariant_exit = false;
>        ch_decision ret;
>        auto_vec <ch_decision, 32> decision;
> @@ -893,7 +893,7 @@ ch_base::copy_headers (function *fun)
>         }
>        /* "Duplicate" all BBs with zero cost following last basic blocks we
>          decided to copy.  */
> -      while (last_win_nheaders < (int)decision.length ()
> +      while (last_win_nheaders < decision.length ()
>              && decision[last_win_nheaders] == ch_possible_zero_cost)
>         {
>           if (dump_file && (dump_flags & TDF_DETAILS))
> diff --git a/gcc/tree-ssa-structalias.cc b/gcc/tree-ssa-structalias.cc
> index a32ef1d5cc0..7adb50a896d 100644
> --- a/gcc/tree-ssa-structalias.cc
> +++ b/gcc/tree-ssa-structalias.cc
> @@ -5925,7 +5925,7 @@ field_must_have_pointers (tree t)
>
>  static bool
>  push_fields_onto_fieldstack (tree type, vec<fieldoff_s> *fieldstack,
> -                            HOST_WIDE_INT offset)
> +                            unsigned HOST_WIDE_INT offset)
>  {
>    tree field;
>    bool empty_p = true;
> @@ -5943,7 +5943,7 @@ push_fields_onto_fieldstack (tree type, vec<fieldoff_s> *fieldstack,
>      if (TREE_CODE (field) == FIELD_DECL)
>        {
>         bool push = false;
> -       HOST_WIDE_INT foff = bitpos_of_field (field);
> +       unsigned HOST_WIDE_INT foff = bitpos_of_field (field);

Can you make bitpos_of_field return unsigned HOST_WIDE_INT then and adjust it
accordingly - it looks for shwi fitting but negative DECL_FIELD_OFFSET
or BIT_OFFSET
are not a thing.

>         tree field_type = TREE_TYPE (field);
>
>         if (!var_can_have_subvars (field)
> @@ -5988,7 +5988,7 @@ push_fields_onto_fieldstack (tree type, vec<fieldoff_s> *fieldstack,
>                 && !must_have_pointers_p
>                 && !pair->must_have_pointers
>                 && !pair->has_unknown_size
> -               && pair->offset + (HOST_WIDE_INT)pair->size == offset + foff)
> +               && pair->offset + pair->size == offset + foff)
>               {
>                 pair->size += tree_to_uhwi (DECL_SIZE (field));
>               }
> diff --git a/gcc/tree-vect-slp.cc b/gcc/tree-vect-slp.cc
> index 9c817de18bd..0bd385f2328 100644
> --- a/gcc/tree-vect-slp.cc
> +++ b/gcc/tree-vect-slp.cc
> @@ -7471,7 +7471,8 @@ vect_prologue_cost_for_slp (slp_tree node,
>        nelt_limit = const_nunits;
>        hash_set<vect_scalar_ops_slice_hash> vector_ops;
>        for (unsigned int i = 0; i < SLP_TREE_NUMBER_OF_VEC_STMTS (node); ++i)
> -       if (!vector_ops.add ({ ops, i * const_nunits, const_nunits }))

So why do we diagnose this case (unsigned int member) but not ...

> +       if (!vector_ops.add
> +           ({ ops, i * (unsigned)const_nunits, (unsigned)const_nunits }))
>           starts.quick_push (i * const_nunits);

... this one - unsigned int function argument?

I think it would be slightly better to do

        {
           unsigned start = (unsigned) const_units * i;
           if (!vector_ops.add ({ ops, start, const_unints }))
             starts.quick_push (start);
        }

to avoid the non-obvious difference between both.

OK with that change.
Richard.

>      }
>    else
> diff --git a/gcc/tree-vect-stmts.cc b/gcc/tree-vect-stmts.cc
> index 45003f762dd..688b8715a15 100644
> --- a/gcc/tree-vect-stmts.cc
> +++ b/gcc/tree-vect-stmts.cc
> @@ -1711,11 +1711,11 @@ vect_truncate_gather_scatter_offset (stmt_vec_info stmt_info,
>      count = max_iters.to_shwi ();
>
>    /* Try scales of 1 and the element size.  */
> -  int scales[] = { 1, vect_get_scalar_dr_size (dr_info) };
> +  unsigned int scales[] = { 1, vect_get_scalar_dr_size (dr_info) };
>    wi::overflow_type overflow = wi::OVF_NONE;
>    for (int i = 0; i < 2; ++i)
>      {
> -      int scale = scales[i];
> +      unsigned int scale = scales[i];
>        widest_int factor;
>        if (!wi::multiple_of_p (wi::to_widest (step), scale, SIGNED, &factor))
>         continue;
> @@ -6528,7 +6528,8 @@ vectorizable_operation (vec_info *vinfo,
>    poly_uint64 nunits_in;
>    poly_uint64 nunits_out;
>    tree vectype_out;
> -  int ncopies, vec_num;
> +  unsigned int ncopies;
> +  int vec_num;
>    int i;
>    vec<tree> vec_oprnds0 = vNULL;
>    vec<tree> vec_oprnds1 = vNULL;
> diff --git a/gcc/configure.ac b/gcc/configure.ac
> index 8a2d2b0438e..23f4884eff9 100644
> --- a/gcc/configure.ac
> +++ b/gcc/configure.ac
> @@ -585,7 +585,6 @@ AC_SUBST(aliasing_flags)
>  # * 'long long'
>  # * variadic macros
>  # * overlong strings
> -# * C++11 narrowing conversions in { }
>  # So, we only use -pedantic if we can disable those warnings.
>
>  # In stage 1, disable -Wformat warnings from old GCCs about new % codes
> @@ -595,7 +594,7 @@ AC_ARG_ENABLE(build-format-warnings,
>  AS_IF([test $enable_build_format_warnings = no],
>        [wf_opt=-Wno-format],[wf_opt=])
>  ACX_PROG_CXX_WARNING_OPTS(
> -       m4_quote(m4_do([-W -Wall -Wno-narrowing -Wwrite-strings ],
> +       m4_quote(m4_do([-W -Wall -Wno-error=narrowing -Wwrite-strings ],
>                        [-Wcast-qual $wf_opt])),
>                        [loose_warn])
>  ACX_PROG_CC_WARNING_OPTS(
> diff --git a/gcc/rtl-ssa/member-fns.inl b/gcc/rtl-ssa/member-fns.inl
> index d39184fb8cd..143c22c8c77 100644
> --- a/gcc/rtl-ssa/member-fns.inl
> +++ b/gcc/rtl-ssa/member-fns.inl
> @@ -41,7 +41,8 @@ access_array_builder::quick_push (access_info *access)
>  inline array_slice<access_info *>
>  access_array_builder::finish ()
>  {
> -  auto num_accesses = obstack_object_size (m_obstack) / sizeof (access_info *);
> +  unsigned num_accesses
> +    = obstack_object_size (m_obstack) / sizeof (access_info *);
>    if (num_accesses == 0)
>      return {};
>
> diff --git a/gcc/configure b/gcc/configure
> index 3d301b6ecd3..5acc42c1e4d 100755
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -7146,7 +7146,6 @@ fi
>  # * 'long long'
>  # * variadic macros
>  # * overlong strings
> -# * C++11 narrowing conversions in { }
>  # So, we only use -pedantic if we can disable those warnings.
>
>  # In stage 1, disable -Wformat warnings from old GCCs about new % codes
> @@ -7170,7 +7169,7 @@ ac_compiler_gnu=$ac_cv_cxx_compiler_gnu
>
>  loose_warn=
>  save_CXXFLAGS="$CXXFLAGS"
> -for real_option in -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual $wf_opt; do
> +for real_option in -W -Wall -Wno-error=narrowing -Wwrite-strings -Wcast-qual $wf_opt; do
>    # Do the check with the no- prefix removed since gcc silently
>    # accepts any -Wno-* option on purpose
>    case $real_option in
> @@ -21406,7 +21405,7 @@ else
>    lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>    lt_status=$lt_dlunknown
>    cat > conftest.$ac_ext <<_LT_EOF
> -#line 21409 "configure"
> +#line 21408 "configure"
>  #include "confdefs.h"
>
>  #if HAVE_DLFCN_H
> @@ -21512,7 +21511,7 @@ else
>    lt_dlunknown=0; lt_dlno_uscore=1; lt_dlneed_uscore=2
>    lt_status=$lt_dlunknown
>    cat > conftest.$ac_ext <<_LT_EOF
> -#line 21515 "configure"
> +#line 21514 "configure"
>  #include "confdefs.h"
>
>  #if HAVE_DLFCN_H
>
> base-commit: 2484ba167e1c4a52d4989b71e1f5d4d27755500f
> --
> 2.46.0
>

On 9/23/24 9:05 AM, Richard Biener wrote:
> On Sat, Sep 21, 2024 at 2:49 AM Jason Merrill <jason@redhat.com> wrote:
>>
>> Tested x86_64-pc-linux-gnu.  OK for trunk?
>>
>> -- 8< --
>>
>> We've been using -Wno-narrowing since gcc 4.7, but at this point narrowing
>> diagnostics seem like a stable part of C++ and we should adjust.
>>
>> This patch changes -Wno-narrowing to -Wno-error=narrowing so that narrowing
>> issues will still not break bootstrap, but we can see them.
>>
>> The rest of the patch fixes the narrowing warnings I see in an
>> x86_64-pc-linux-gnu bootstrap.  In most of the cases, by adjusting the types
>> of various declarations so that we store the values in the same types we
>> compute them in, which seems worthwhile anyway.  This also allowed us to
>> remove a few -Wsign-compare casts.
>>
>> The one place I didn't see how best to do this was in
>> vect_prologue_cost_for_slp: changing const_nunits to unsigned int broke the
>> call to TYPE_VECTOR_SUBPARTS (vectype).is_constant (&const_nunits), since
>> poly_uint64.is_constant wants a pointer to unsigned HOST_WIDE_INT.  So I
>> added casts in that one place.  Not too bad, I think.
>>
>> +       unsigned HOST_WIDE_INT foff = bitpos_of_field (field);
> 
> Can you make bitpos_of_field return unsigned HOST_WIDE_INT then and adjust it
> accordingly - it looks for shwi fitting but negative DECL_FIELD_OFFSET
> or BIT_OFFSET are not a thing.

So, like the attached?

>> @@ -7471,7 +7471,8 @@ vect_prologue_cost_for_slp (slp_tree node,
>>         nelt_limit = const_nunits;
>>         hash_set<vect_scalar_ops_slice_hash> vector_ops;
>>         for (unsigned int i = 0; i < SLP_TREE_NUMBER_OF_VEC_STMTS (node); ++i)
>> -       if (!vector_ops.add ({ ops, i * const_nunits, const_nunits }))
> 
> So why do we diagnose this case (unsigned int member) but not ...
> 
>> +       if (!vector_ops.add
>> +           ({ ops, i * (unsigned)const_nunits, (unsigned)const_nunits }))
>>            starts.quick_push (i * const_nunits);
> 
> ... this one - unsigned int function argument?

Because the former is in { }, and the latter isn't; narrowing 
conversions are only ill-formed within { }.

> I think it would be slightly better to do
> 
>          {
>             unsigned start = (unsigned) const_units * i;
>             if (!vector_ops.add ({ ops, start, const_unints }))
>               starts.quick_push (start);
>          }
> 
> to avoid the non-obvious difference between both.

We'd still need the cast for the third element, but now I notice we can 
use nelt_limit instead since it just got the same value.

So, OK with this supplemental patch?

On Mon, Sep 23, 2024 at 3:41 PM Jason Merrill <jason@redhat.com> wrote:
>
> On 9/23/24 9:05 AM, Richard Biener wrote:
> > On Sat, Sep 21, 2024 at 2:49 AM Jason Merrill <jason@redhat.com> wrote:
> >>
> >> Tested x86_64-pc-linux-gnu.  OK for trunk?
> >>
> >> -- 8< --
> >>
> >> We've been using -Wno-narrowing since gcc 4.7, but at this point narrowing
> >> diagnostics seem like a stable part of C++ and we should adjust.
> >>
> >> This patch changes -Wno-narrowing to -Wno-error=narrowing so that narrowing
> >> issues will still not break bootstrap, but we can see them.
> >>
> >> The rest of the patch fixes the narrowing warnings I see in an
> >> x86_64-pc-linux-gnu bootstrap.  In most of the cases, by adjusting the types
> >> of various declarations so that we store the values in the same types we
> >> compute them in, which seems worthwhile anyway.  This also allowed us to
> >> remove a few -Wsign-compare casts.
> >>
> >> The one place I didn't see how best to do this was in
> >> vect_prologue_cost_for_slp: changing const_nunits to unsigned int broke the
> >> call to TYPE_VECTOR_SUBPARTS (vectype).is_constant (&const_nunits), since
> >> poly_uint64.is_constant wants a pointer to unsigned HOST_WIDE_INT.  So I
> >> added casts in that one place.  Not too bad, I think.
> >>
> >> +       unsigned HOST_WIDE_INT foff = bitpos_of_field (field);
> >
> > Can you make bitpos_of_field return unsigned HOST_WIDE_INT then and adjust it
> > accordingly - it looks for shwi fitting but negative DECL_FIELD_OFFSET
> > or BIT_OFFSET are not a thing.
>
> So, like the attached?
>
> >> @@ -7471,7 +7471,8 @@ vect_prologue_cost_for_slp (slp_tree node,
> >>         nelt_limit = const_nunits;
> >>         hash_set<vect_scalar_ops_slice_hash> vector_ops;
> >>         for (unsigned int i = 0; i < SLP_TREE_NUMBER_OF_VEC_STMTS (node); ++i)
> >> -       if (!vector_ops.add ({ ops, i * const_nunits, const_nunits }))
> >
> > So why do we diagnose this case (unsigned int member) but not ...
> >
> >> +       if (!vector_ops.add
> >> +           ({ ops, i * (unsigned)const_nunits, (unsigned)const_nunits }))
> >>            starts.quick_push (i * const_nunits);
> >
> > ... this one - unsigned int function argument?
>
> Because the former is in { }, and the latter isn't; narrowing
> conversions are only ill-formed within { }.
>
> > I think it would be slightly better to do
> >
> >          {
> >             unsigned start = (unsigned) const_units * i;
> >             if (!vector_ops.add ({ ops, start, const_unints }))
> >               starts.quick_push (start);
> >          }
> >
> > to avoid the non-obvious difference between both.
>
> We'd still need the cast for the third element, but now I notice we can
> use nelt_limit instead since it just got the same value.
>
> So, OK with this supplemental patch?

OK.

Thanks,
Richard.

On Tue, Sep 24, 2024 at 2:41 AM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Mon, Sep 23, 2024 at 3:41 PM Jason Merrill <jason@redhat.com> wrote:
> >
> > On 9/23/24 9:05 AM, Richard Biener wrote:
> > > On Sat, Sep 21, 2024 at 2:49 AM Jason Merrill <jason@redhat.com> wrote:
> > >>
> > >> Tested x86_64-pc-linux-gnu.  OK for trunk?
> > >>
> > >> -- 8< --
> > >>
> > >> We've been using -Wno-narrowing since gcc 4.7, but at this point narrowing
> > >> diagnostics seem like a stable part of C++ and we should adjust.
> > >>
> > >> This patch changes -Wno-narrowing to -Wno-error=narrowing so that narrowing
> > >> issues will still not break bootstrap, but we can see them.
> > >>
> > >> The rest of the patch fixes the narrowing warnings I see in an
> > >> x86_64-pc-linux-gnu bootstrap.  In most of the cases, by adjusting the types
> > >> of various declarations so that we store the values in the same types we
> > >> compute them in, which seems worthwhile anyway.  This also allowed us to
> > >> remove a few -Wsign-compare casts.
> > >>
> > >> The one place I didn't see how best to do this was in
> > >> vect_prologue_cost_for_slp: changing const_nunits to unsigned int broke the
> > >> call to TYPE_VECTOR_SUBPARTS (vectype).is_constant (&const_nunits), since
> > >> poly_uint64.is_constant wants a pointer to unsigned HOST_WIDE_INT.  So I
> > >> added casts in that one place.  Not too bad, I think.
> > >>
> > >> +       unsigned HOST_WIDE_INT foff = bitpos_of_field (field);
> > >
> > > Can you make bitpos_of_field return unsigned HOST_WIDE_INT then and adjust it
> > > accordingly - it looks for shwi fitting but negative DECL_FIELD_OFFSET
> > > or BIT_OFFSET are not a thing.
> >
> > So, like the attached?
> >
> > >> @@ -7471,7 +7471,8 @@ vect_prologue_cost_for_slp (slp_tree node,
> > >>         nelt_limit = const_nunits;
> > >>         hash_set<vect_scalar_ops_slice_hash> vector_ops;
> > >>         for (unsigned int i = 0; i < SLP_TREE_NUMBER_OF_VEC_STMTS (node); ++i)
> > >> -       if (!vector_ops.add ({ ops, i * const_nunits, const_nunits }))
> > >
> > > So why do we diagnose this case (unsigned int member) but not ...
> > >
> > >> +       if (!vector_ops.add
> > >> +           ({ ops, i * (unsigned)const_nunits, (unsigned)const_nunits }))
> > >>            starts.quick_push (i * const_nunits);
> > >
> > > ... this one - unsigned int function argument?
> >
> > Because the former is in { }, and the latter isn't; narrowing
> > conversions are only ill-formed within { }.
> >
> > > I think it would be slightly better to do
> > >
> > >          {
> > >             unsigned start = (unsigned) const_units * i;
> > >             if (!vector_ops.add ({ ops, start, const_unints }))
> > >               starts.quick_push (start);
> > >          }
> > >
> > > to avoid the non-obvious difference between both.
> >
> > We'd still need the cast for the third element, but now I notice we can
> > use nelt_limit instead since it just got the same value.
> >
> > So, OK with this supplemental patch?
>
> OK.
>
> Thanks,
> Richard.

Note that libcpp uses -Wno-narrowing, too; perhaps it's worth making
this change there, as well?