Re: [PATCH] Remove poly_int_pod
Jan-Benedict Glaw writes:
> Hi Richard,
>
> On Thu, 2023-09-28 10:55:46 +0100, Richard Sandiford
> wrote:
>> poly_int was written before the switch to C++11 and so couldn't
>> use explicit default constructors. This led to an awkward split
>> between poly_int_pod and poly_int. poly_int simply inherited from
>> poly_int_pod and added constructors, with the argumentless constructor
>> having an empty body. But inheritance meant that poly_int had to
>> repeat the assignment operators from poly_int_pod (again, no C++11,
>> so no "using" to inherit base-class implementations).
> [...]
>
> I haven't bisected it, but I guess your patch caused this:
>
> [all 2023-10-02 06:59:02]
> /var/lib/laminar/run/gcc-local/75/local-toolchain-install/bin/g++ -std=c++11
> -fno-PIE -c -g -O2 -DIN_GCC-fno-exceptions -fno-rtti
> -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings
> -Wcast-qual -Wmissing-format-attribute -Wconditionally-supported
> -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros
> -Wno-overlength-strings -Werror -fno-common -DHAVE_CONFIG_H -fno-PIE -I. -I.
> -I../../gcc/gcc -I../../gcc/gcc/. -I../../gcc/gcc/../include
> -I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libcody
> -I../../gcc/gcc/../libdecnumber -I../../gcc/gcc/../libdecnumber/bid
> -I../libdecnumber -I../../gcc/gcc/../libbacktrace -o rtl-tests.o -MT
> rtl-tests.o -MMD -MP -MF ./.deps/rtl-tests.TPo ../../gcc/gcc/rtl-tests.cc
> [all 2023-10-02 06:59:04] In file included from ../../gcc/gcc/coretypes.h:480,
> [all 2023-10-02 06:59:04] from ../../gcc/gcc/rtl-tests.cc:22:
> [all 2023-10-02 06:59:04] ../../gcc/gcc/poly-int.h: In instantiation of
> 'constexpr poly_int::poly_int(poly_int_full, const Cs& ...) [with Cs =
> {int, int}; unsigned int N = 1; C = long int]':
> [all 2023-10-02 06:59:04] ../../gcc/gcc/poly-int.h:439:13: required from
> here
> [all 2023-10-02 06:59:04] ../../gcc/gcc/rtl-tests.cc:249:25: in 'constexpr'
> expansion of 'poly_int<1, long int>(1, 1)'
> [all 2023-10-02 06:59:04] ../../gcc/gcc/poly-int.h:453:5: error: too many
> initializers for 'long int [1]'
> [all 2023-10-02 06:59:04] 453 | : coeffs { (typename
> poly_coeff_traits::
> [all 2023-10-02 06:59:04] |
> ^
> [all 2023-10-02 06:59:04] 454 | template init_cast::type
> (cs))... } {}
> [all 2023-10-02 06:59:04] |
> ~~~
> [all 2023-10-02 06:59:04] make[1]: *** [Makefile:1188: rtl-tests.o] Error 1
> [all 2023-10-02 06:59:04] make[1]: Leaving directory
> '/var/lib/laminar/run/gcc-local/75/toolchain-build/gcc'
> [all 2023-10-02 06:59:05] make: *** [Makefile:4993: all-gcc] Error 2
>
>
> (Full build log at
> http://toolchain.lug-owl.de/laminar/jobs/gcc-local/75 . That's in a
> Docker container on amd64-linux with the host gcc being at fairly new
> at basepoints/gcc-14-3827-g30e6ee07458)
Yeah, this was PR111642. I pushed a fix this morning.
Thanks,
Richard
Re: [PATCH] Remove poly_int_pod
Hi Richard,
On Thu, 2023-09-28 10:55:46 +0100, Richard Sandiford
wrote:
> poly_int was written before the switch to C++11 and so couldn't
> use explicit default constructors. This led to an awkward split
> between poly_int_pod and poly_int. poly_int simply inherited from
> poly_int_pod and added constructors, with the argumentless constructor
> having an empty body. But inheritance meant that poly_int had to
> repeat the assignment operators from poly_int_pod (again, no C++11,
> so no "using" to inherit base-class implementations).
[...]
I haven't bisected it, but I guess your patch caused this:
[all 2023-10-02 06:59:02]
/var/lib/laminar/run/gcc-local/75/local-toolchain-install/bin/g++ -std=c++11
-fno-PIE -c -g -O2 -DIN_GCC-fno-exceptions -fno-rtti
-fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings
-Wcast-qual -Wmissing-format-attribute -Wconditionally-supported
-Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros
-Wno-overlength-strings -Werror -fno-common -DHAVE_CONFIG_H -fno-PIE -I. -I.
-I../../gcc/gcc -I../../gcc/gcc/. -I../../gcc/gcc/../include
-I../../gcc/gcc/../libcpp/include -I../../gcc/gcc/../libcody
-I../../gcc/gcc/../libdecnumber -I../../gcc/gcc/../libdecnumber/bid
-I../libdecnumber -I../../gcc/gcc/../libbacktrace -o rtl-tests.o -MT
rtl-tests.o -MMD -MP -MF ./.deps/rtl-tests.TPo ../../gcc/gcc/rtl-tests.cc
[all 2023-10-02 06:59:04] In file included from ../../gcc/gcc/coretypes.h:480,
[all 2023-10-02 06:59:04] from ../../gcc/gcc/rtl-tests.cc:22:
[all 2023-10-02 06:59:04] ../../gcc/gcc/poly-int.h: In instantiation of
'constexpr poly_int::poly_int(poly_int_full, const Cs& ...) [with Cs =
{int, int}; unsigned int N = 1; C = long int]':
[all 2023-10-02 06:59:04] ../../gcc/gcc/poly-int.h:439:13: required from here
[all 2023-10-02 06:59:04] ../../gcc/gcc/rtl-tests.cc:249:25: in 'constexpr'
expansion of 'poly_int<1, long int>(1, 1)'
[all 2023-10-02 06:59:04] ../../gcc/gcc/poly-int.h:453:5: error: too many
initializers for 'long int [1]'
[all 2023-10-02 06:59:04] 453 | : coeffs { (typename poly_coeff_traits::
[all 2023-10-02 06:59:04] | ^
[all 2023-10-02 06:59:04] 454 | template init_cast::type
(cs))... } {}
[all 2023-10-02 06:59:04] |
~~~
[all 2023-10-02 06:59:04] make[1]: *** [Makefile:1188: rtl-tests.o] Error 1
[all 2023-10-02 06:59:04] make[1]: Leaving directory
'/var/lib/laminar/run/gcc-local/75/toolchain-build/gcc'
[all 2023-10-02 06:59:05] make: *** [Makefile:4993: all-gcc] Error 2
(Full build log at
http://toolchain.lug-owl.de/laminar/jobs/gcc-local/75 . That's in a
Docker container on amd64-linux with the host gcc being at fairly new
at basepoints/gcc-14-3827-g30e6ee07458)
MfG, JBG
--
signature.asc
Description: PGP signature
Re: [PATCH] Remove poly_int_pod
On Fri, Sep 29, 2023 at 08:31:47AM +0200, Richard Biener wrote:
> > IIRC the primary reason we settled on gcc-4.8.x was RHEL7/Centos7. With
> > RHEL 7 approaching EOL moving the baseline forward would seem to make sense.
> >
> > I'd want to know if this affects folks using SuSE's enterprise distro
> > before actually making the change, but I'm broadly in favor of moving
> > forward it it's not going to have a major impact on users that are using
> > enterprise distros.
>
> We're thinking of making GCC 13 the last major release to officially
> build for SLE12 which
> also uses GCC 4.8, so we'd be fine with doing this for GCC 14.
We'd need to figure out what to do with the OS on gcc{110,112,135} on CFarm,
those are all CentOS 7.9 with gcc 4.8.5. Sure, one possibility would be
to install some DTS gcc if CentOS has one somewhere (though looking around,
/opt/at12.0 already has there gcc 8 from IBM Advanced Toolchain 12).
Jakub
Re: [PATCH] Remove poly_int_pod
On Thu, Sep 28, 2023 at 9:10 PM Jeff Law wrote: > > > > On 9/28/23 11:26, Jason Merrill wrote: > > On 9/28/23 05:55, Richard Sandiford wrote: > >> poly_int was written before the switch to C++11 and so couldn't > >> use explicit default constructors. This led to an awkward split > >> between poly_int_pod and poly_int. poly_int simply inherited from > >> poly_int_pod and added constructors, with the argumentless constructor > >> having an empty body. But inheritance meant that poly_int had to > >> repeat the assignment operators from poly_int_pod (again, no C++11, > >> so no "using" to inherit base-class implementations). > >> > >> All that goes away if we switch to using default constructors. > >> > >> The main complication is ensuring that braced initialisation still > >> gives a constexpr, so that static variables can be initialised without > >> runtime code. The two problems here are: > >> > >> (1) When initialising a poly_int with fewer than N > >> coefficients, the other coefficients need to be a zero of > >> the same precision as the explicit coefficients. This was > >> previously done in a for loop using wi::ints_for<...>::zero, > >> but C++11 constexpr constructors can't have function bodies. > >> The patch instead uses a series of delegated initialisers to > >> fill in the implicit coefficients. > > > > Perhaps it's time to update the bootstrap requirement to C++14 (i.e. GCC > > 5, from eight years ago). Not that this would affect this particular > > patch. > IIRC the primary reason we settled on gcc-4.8.x was RHEL7/Centos7. With > RHEL 7 approaching EOL moving the baseline forward would seem to make sense. > > I'd want to know if this affects folks using SuSE's enterprise distro > before actually making the change, but I'm broadly in favor of moving > forward it it's not going to have a major impact on users that are using > enterprise distros. We're thinking of making GCC 13 the last major release to officially build for SLE12 which also uses GCC 4.8, so we'd be fine with doing this for GCC 14. Richard. > > jeff
Re: [PATCH] Remove poly_int_pod
On 9/28/23 11:26, Jason Merrill wrote: On 9/28/23 05:55, Richard Sandiford wrote: poly_int was written before the switch to C++11 and so couldn't use explicit default constructors. This led to an awkward split between poly_int_pod and poly_int. poly_int simply inherited from poly_int_pod and added constructors, with the argumentless constructor having an empty body. But inheritance meant that poly_int had to repeat the assignment operators from poly_int_pod (again, no C++11, so no "using" to inherit base-class implementations). All that goes away if we switch to using default constructors. The main complication is ensuring that braced initialisation still gives a constexpr, so that static variables can be initialised without runtime code. The two problems here are: (1) When initialising a poly_int with fewer than N coefficients, the other coefficients need to be a zero of the same precision as the explicit coefficients. This was previously done in a for loop using wi::ints_for<...>::zero, but C++11 constexpr constructors can't have function bodies. The patch instead uses a series of delegated initialisers to fill in the implicit coefficients. Perhaps it's time to update the bootstrap requirement to C++14 (i.e. GCC 5, from eight years ago). Not that this would affect this particular patch. IIRC the primary reason we settled on gcc-4.8.x was RHEL7/Centos7. With RHEL 7 approaching EOL moving the baseline forward would seem to make sense. I'd want to know if this affects folks using SuSE's enterprise distro before actually making the change, but I'm broadly in favor of moving forward it it's not going to have a major impact on users that are using enterprise distros. jeff
Re: [PATCH] Remove poly_int_pod
On 9/28/23 05:55, Richard Sandiford wrote: poly_int was written before the switch to C++11 and so couldn't use explicit default constructors. This led to an awkward split between poly_int_pod and poly_int. poly_int simply inherited from poly_int_pod and added constructors, with the argumentless constructor having an empty body. But inheritance meant that poly_int had to repeat the assignment operators from poly_int_pod (again, no C++11, so no "using" to inherit base-class implementations). All that goes away if we switch to using default constructors. The main complication is ensuring that braced initialisation still gives a constexpr, so that static variables can be initialised without runtime code. The two problems here are: (1) When initialising a poly_int with fewer than N coefficients, the other coefficients need to be a zero of the same precision as the explicit coefficients. This was previously done in a for loop using wi::ints_for<...>::zero, but C++11 constexpr constructors can't have function bodies. The patch instead uses a series of delegated initialisers to fill in the implicit coefficients. Perhaps it's time to update the bootstrap requirement to C++14 (i.e. GCC 5, from eight years ago). Not that this would affect this particular patch. Jason
Re: [PATCH] Remove poly_int_pod
On Thu, Sep 28, 2023 at 10:55:46AM +0100, Richard Sandiford wrote: > Tested on aarch64-linux-gnu & x86_64-linux-gnu. Also tested with > Jakub's vec.h patch with the static_asserts uncommented; there were > no errors from poly_int-related stuff. OK to install? LGTM (mostly as the general idea, but didn't see anything wrong in the patch either), please give others a day or so to comment though. Jakub
[PATCH] Remove poly_int_pod
poly_int was written before the switch to C++11 and so couldn't
use explicit default constructors. This led to an awkward split
between poly_int_pod and poly_int. poly_int simply inherited from
poly_int_pod and added constructors, with the argumentless constructor
having an empty body. But inheritance meant that poly_int had to
repeat the assignment operators from poly_int_pod (again, no C++11,
so no "using" to inherit base-class implementations).
All that goes away if we switch to using default constructors.
The main complication is ensuring that braced initialisation still
gives a constexpr, so that static variables can be initialised without
runtime code. The two problems here are:
(1) When initialising a poly_int with fewer than N
coefficients, the other coefficients need to be a zero of
the same precision as the explicit coefficients. This was
previously done in a for loop using wi::ints_for<...>::zero,
but C++11 constexpr constructors can't have function bodies.
The patch instead uses a series of delegated initialisers to
fill in the implicit coefficients.
(2) The initialisation in:
void f(int x) {
unsigned int foo {x};
}
produces the warning:
warning: narrowing conversion of 'x' from 'int' to 'unsigned int'
[-Wnarrowing]
whereas:
void f(int x) {
unsigned int foo = x;
}
does not. So switching to direct initialisation of the coeffs array
would mean that:
poly_uin64_t x = 0;
would trigger a warning for using 0 rather than 0u. That seemed
overly pedantic, so the patch adds explicit casts to the constructor.
The complication is to do that without adding extra code to
wide-int versions. The patch uses a new init_cast type for that.
Tested on aarch64-linux-gnu & x86_64-linux-gnu. Also tested with
Jakub's vec.h patch with the static_asserts uncommented; there were
no errors from poly_int-related stuff. OK to install?
Richard
gcc/
* poly-int.h (poly_int_pod): Delete.
(poly_coeff_traits::init_cast): New type.
(poly_int_full, poly_int_hungry, poly_int_fullness): New structures.
(poly_int): Replace constructors that take 1 and 2 coefficients with
a general one that takes an arbitrary number of coefficients.
Delegate initialization to two new private constructors, one of
which uses the coefficients as-is and one of which adds an extra
zero of the appropriate type (and precision, where applicable).
(gt_ggc_mx, gt_pch_nx): Operate on poly_ints rather than poly_int_pods.
* poly-int-types.h (poly_uint16_pod, poly_int64_pod, poly_uint64_pod)
(poly_offset_int_pod, poly_wide_int_pod, poly_widest_int_pod): Delete.
* gengtype.cc (main): Don't register poly_int64_pod.
* calls.cc (initialize_argument_information): Use poly_int rather
than poly_int_pod.
(combine_pending_stack_adjustment_and_call): Likewise.
* config/aarch64/aarch64.cc (pure_scalable_type_info): Likewise.
* data-streamer.h (bp_unpack_poly_value): Likewise.
* dwarf2cfi.cc (struct dw_trace_info): Likewise.
(struct queued_reg_save): Likewise.
* dwarf2out.h (struct dw_cfa_location): Likewise.
* emit-rtl.h (struct incoming_args): Likewise.
(struct rtl_data): Likewise.
* expr.cc (get_bit_range): Likewise.
(get_inner_reference): Likewise.
* expr.h (get_bit_range): Likewise.
* fold-const.cc (split_address_to_core_and_offset): Likewise.
(ptr_difference_const): Likewise.
* fold-const.h (ptr_difference_const): Likewise.
* function.cc (try_fit_stack_local): Likewise.
(instantiate_new_reg): Likewise.
* function.h (struct expr_status): Likewise.
(struct args_size): Likewise.
* genmodes.cc (ZERO_COEFFS): Likewise.
(mode_size_inline): Likewise.
(mode_nunits_inline): Likewise.
(emit_mode_precision): Likewise.
(emit_mode_size): Likewise.
(emit_mode_nunits): Likewise.
* gimple-fold.cc (get_base_constructor): Likewise.
* gimple-ssa-store-merging.cc (struct symbolic_number): Likewise.
* inchash.h (class hash): Likewise.
* ipa-modref-tree.cc (modref_access_node::dump): Likewise.
* ipa-modref.cc (modref_access_analysis::merge_call_side_effects):
Likewise.
* ira-int.h (ira_spilled_reg_stack_slot): Likewise.
* lra-eliminations.cc (self_elim_offsets): Likewise.
* machmode.h (mode_size, mode_precision, mode_nunits): Likewise.
* omp-low.cc (omplow_simd_context): Likewise.
* pretty-print.cc (pp_wide_integer): Likewise.
* pretty-print.h (pp_wide_integer): Likewise.
* reload.cc (struct decomposition): Likewise.
* reload.h (struct reload): Likewise.
* reload1.cc (spill_stack_slot_width): Likewise.
(struct elim_table): Likewise.
