Re: [PATCH] ipa: Convert lattices from pure array to vector (PR 113476)
> On Tue, Feb 13 2024, Martin Jambor wrote: > > On Mon, Feb 12 2024, Jan Hubicka wrote: > >>> Believe it or not, even though I have re-worked the internals of the > >>> lattices completely, the array itself is older than my involvement with > >>> GCC (or at least with ipa-cp.c ;-). > >>> > >>> So it being an array and not a vector is historical coincidence, as far > >>> as I am concerned :-). But that may be the reason, or because vector > >>> macros at that time looked scary, or perhaps the initialization by > >>> XCNEWVEC zeroing everything out was considered attractive (I kind of > >>> like that but constructors would probably be cleaner), I don't know. > >> > >> If your class is no longer a POD, then the clearing before construcion > >> is dead and GCC may optimize it out. So fixing this may solve some > >> surprised in foreseable future when we will try to compile older GCC's > >> with newer ones. > >> > > > > That's a good point. I'll prepare a patch converting the whole thing to > > use constructors and vectors. > > > > In PR 113476 we have discovered that ipcp_param_lattices is no longer > a POD and should be destructed. In a follow-up discussion it > transpired that their initialization done by memsetting their backing > memory to zero is also invalid because now any write there before > construction can be considered dead. Plus that having them in an > array is a little bit old-school and does not get the extra checking > offered by vector along with automatic construction and destruction > when necessary. > > So this patch converts the array to a vector. That however means that > ipcp_param_lattices cannot be just a forward declared type but must be > known to all code that deal with ipa_node_params and thus to all code > that includes ipa-prop.h. Therefore I have moved ipcp_param_lattices > and the type it depends on to a new header ipa-cp.h which now > ipa-prop.h depends on. Because we have the (IMHO not a very wise) > rule that headers don't include what they need themselves, I had to > add inclusions of ipa-cp.h and sreal.h (on which it depends) to very > many files, which made the patch rather ugly. > > Bootstrapped and tested on x86_64-linux. I also had it checked by our > script which builds more than a hundred of cross-compilers, so other > targets are hopefully also fine. > > OK for master? > > Martin > > > gcc/lto/ChangeLog: > > 2024-02-16 Martin Jambor > > * lto-common.cc: Include sreal.h and ipa-cp.h. > * lto-partition.cc: Include ipa-cp.h, move inclusion of sreal higher. > * lto.cc: Include sreal.h and ipa-cp.h. > > gcc/ChangeLog: > > 2024-02-16 Martin Jambor > > * ipa-prop.h (ipa_node_params): Convert lattices to a vector, adjust > initializers in the contructor. > (ipa_node_params::~ipa_node_params): Release lattices as a vector. > * ipa-cp.h: New file. > * ipa-cp.cc: Include sreal.h and ipa-cp.h. > (ipcp_value_source): Move to ipa-cp.h. > (ipcp_value_base): Likewise. > (ipcp_value): Likewise. > (ipcp_lattice): Likewise. > (ipcp_agg_lattice): Likewise. > (ipcp_bits_lattice): Likewise. > (ipcp_vr_lattice): Likewise. > (ipcp_param_lattices): Likewise. > (ipa_get_parm_lattices): Remove assert latticess is non-NULL). > (ipa_value_from_jfunc): Adjust a check for empty lattices. > (ipa_context_from_jfunc): Likewise. > (ipa_agg_value_from_jfunc): Likewise. > (merge_agg_lats_step): Do not memset new aggregate lattices to zero. > (ipcp_propagate_stage): Allocate lattices in a vector as opposed to > just in contiguous memory. > (ipcp_store_vr_results): Adjust a check for empty lattices. > * auto-profile.cc: Include sreal.h and ipa-cp.h. > * cgraph.cc: Likewise. > * cgraphclones.cc: Likewise. > * cgraphunit.cc: Likewise. > * config/aarch64/aarch64.cc: Likewise. > * config/i386/i386-builtins.cc: Likewise. > * config/i386/i386-expand.cc: Likewise. > * config/i386/i386-features.cc: Likewise. > * config/i386/i386-options.cc: Likewise. > * config/i386/i386.cc: Likewise. > * config/rs6000/rs6000.cc: Likewise. > * config/s390/s390.cc: Likewise. > * gengtype.cc (open_base_files): Added sreal.h and ipa-cp.h to the > files to be included in gtype-desc.cc. > * gimple-range-fold.cc: Include sreal.h and ipa-cp.h. > * ipa-devirt.cc: Likewise. > * ipa-fnsummary.cc: Likewise. > * ipa-icf.cc: Likewise. > * ipa-inline-analysis.cc: Likewise. > * ipa-inline-transform.cc: Likewise. > * ipa-inline.cc: Include ipa-cp.h, move inclusion of sreal.h higher. > * ipa-modref.cc: Include sreal.h and ipa-cp.h. > * ipa-param-manipulation.cc: Likewise. > * ipa-predicate.cc: Likewise. > * ipa-profile.cc: Likewise. > * ipa-prop.cc: Likewise. > (ipa_node_params_t::duplicate): Assert new lattices
Re: [PATCH] ipa: Convert lattices from pure array to vector (PR 113476)
On Mon, 19 Feb 2024, Martin Jambor wrote: > On Tue, Feb 13 2024, Martin Jambor wrote: > > On Mon, Feb 12 2024, Jan Hubicka wrote: > >>> Believe it or not, even though I have re-worked the internals of the > >>> lattices completely, the array itself is older than my involvement with > >>> GCC (or at least with ipa-cp.c ;-). > >>> > >>> So it being an array and not a vector is historical coincidence, as far > >>> as I am concerned :-). But that may be the reason, or because vector > >>> macros at that time looked scary, or perhaps the initialization by > >>> XCNEWVEC zeroing everything out was considered attractive (I kind of > >>> like that but constructors would probably be cleaner), I don't know. > >> > >> If your class is no longer a POD, then the clearing before construcion > >> is dead and GCC may optimize it out. So fixing this may solve some > >> surprised in foreseable future when we will try to compile older GCC's > >> with newer ones. > >> > > > > That's a good point. I'll prepare a patch converting the whole thing to > > use constructors and vectors. > > > > In PR 113476 we have discovered that ipcp_param_lattices is no longer > a POD and should be destructed. In a follow-up discussion it > transpired that their initialization done by memsetting their backing > memory to zero is also invalid because now any write there before > construction can be considered dead. Plus that having them in an > array is a little bit old-school and does not get the extra checking > offered by vector along with automatic construction and destruction > when necessary. > > So this patch converts the array to a vector. That however means that > ipcp_param_lattices cannot be just a forward declared type but must be > known to all code that deal with ipa_node_params and thus to all code > that includes ipa-prop.h. Therefore I have moved ipcp_param_lattices > and the type it depends on to a new header ipa-cp.h which now > ipa-prop.h depends on. Because we have the (IMHO not a very wise) > rule that headers don't include what they need themselves, I had to > add inclusions of ipa-cp.h and sreal.h (on which it depends) to very > many files, which made the patch rather ugly. > > Bootstrapped and tested on x86_64-linux. I also had it checked by our > script which builds more than a hundred of cross-compilers, so other > targets are hopefully also fine. > > OK for master? LGTM. Thanks, Richard. > Martin > > > gcc/lto/ChangeLog: > > 2024-02-16 Martin Jambor > > * lto-common.cc: Include sreal.h and ipa-cp.h. > * lto-partition.cc: Include ipa-cp.h, move inclusion of sreal higher. > * lto.cc: Include sreal.h and ipa-cp.h. > > gcc/ChangeLog: > > 2024-02-16 Martin Jambor > > * ipa-prop.h (ipa_node_params): Convert lattices to a vector, adjust > initializers in the contructor. > (ipa_node_params::~ipa_node_params): Release lattices as a vector. > * ipa-cp.h: New file. > * ipa-cp.cc: Include sreal.h and ipa-cp.h. > (ipcp_value_source): Move to ipa-cp.h. > (ipcp_value_base): Likewise. > (ipcp_value): Likewise. > (ipcp_lattice): Likewise. > (ipcp_agg_lattice): Likewise. > (ipcp_bits_lattice): Likewise. > (ipcp_vr_lattice): Likewise. > (ipcp_param_lattices): Likewise. > (ipa_get_parm_lattices): Remove assert latticess is non-NULL). > (ipa_value_from_jfunc): Adjust a check for empty lattices. > (ipa_context_from_jfunc): Likewise. > (ipa_agg_value_from_jfunc): Likewise. > (merge_agg_lats_step): Do not memset new aggregate lattices to zero. > (ipcp_propagate_stage): Allocate lattices in a vector as opposed to > just in contiguous memory. > (ipcp_store_vr_results): Adjust a check for empty lattices. > * auto-profile.cc: Include sreal.h and ipa-cp.h. > * cgraph.cc: Likewise. > * cgraphclones.cc: Likewise. > * cgraphunit.cc: Likewise. > * config/aarch64/aarch64.cc: Likewise. > * config/i386/i386-builtins.cc: Likewise. > * config/i386/i386-expand.cc: Likewise. > * config/i386/i386-features.cc: Likewise. > * config/i386/i386-options.cc: Likewise. > * config/i386/i386.cc: Likewise. > * config/rs6000/rs6000.cc: Likewise. > * config/s390/s390.cc: Likewise. > * gengtype.cc (open_base_files): Added sreal.h and ipa-cp.h to the > files to be included in gtype-desc.cc. > * gimple-range-fold.cc: Include sreal.h and ipa-cp.h. > * ipa-devirt.cc: Likewise. > * ipa-fnsummary.cc: Likewise. > * ipa-icf.cc: Likewise. > * ipa-inline-analysis.cc: Likewise. > * ipa-inline-transform.cc: Likewise. > * ipa-inline.cc: Include ipa-cp.h, move inclusion of sreal.h higher. > * ipa-modref.cc: Include sreal.h and ipa-cp.h. > * ipa-param-manipulation.cc: Likewise. > * ipa-predicate.cc: Likewise. > * ipa-profile.cc: Likewise. > * ipa-prop.cc:
[PATCH] ipa: Convert lattices from pure array to vector (PR 113476)
On Tue, Feb 13 2024, Martin Jambor wrote: > On Mon, Feb 12 2024, Jan Hubicka wrote: >>> Believe it or not, even though I have re-worked the internals of the >>> lattices completely, the array itself is older than my involvement with >>> GCC (or at least with ipa-cp.c ;-). >>> >>> So it being an array and not a vector is historical coincidence, as far >>> as I am concerned :-). But that may be the reason, or because vector >>> macros at that time looked scary, or perhaps the initialization by >>> XCNEWVEC zeroing everything out was considered attractive (I kind of >>> like that but constructors would probably be cleaner), I don't know. >> >> If your class is no longer a POD, then the clearing before construcion >> is dead and GCC may optimize it out. So fixing this may solve some >> surprised in foreseable future when we will try to compile older GCC's >> with newer ones. >> > > That's a good point. I'll prepare a patch converting the whole thing to > use constructors and vectors. > In PR 113476 we have discovered that ipcp_param_lattices is no longer a POD and should be destructed. In a follow-up discussion it transpired that their initialization done by memsetting their backing memory to zero is also invalid because now any write there before construction can be considered dead. Plus that having them in an array is a little bit old-school and does not get the extra checking offered by vector along with automatic construction and destruction when necessary. So this patch converts the array to a vector. That however means that ipcp_param_lattices cannot be just a forward declared type but must be known to all code that deal with ipa_node_params and thus to all code that includes ipa-prop.h. Therefore I have moved ipcp_param_lattices and the type it depends on to a new header ipa-cp.h which now ipa-prop.h depends on. Because we have the (IMHO not a very wise) rule that headers don't include what they need themselves, I had to add inclusions of ipa-cp.h and sreal.h (on which it depends) to very many files, which made the patch rather ugly. Bootstrapped and tested on x86_64-linux. I also had it checked by our script which builds more than a hundred of cross-compilers, so other targets are hopefully also fine. OK for master? Martin gcc/lto/ChangeLog: 2024-02-16 Martin Jambor * lto-common.cc: Include sreal.h and ipa-cp.h. * lto-partition.cc: Include ipa-cp.h, move inclusion of sreal higher. * lto.cc: Include sreal.h and ipa-cp.h. gcc/ChangeLog: 2024-02-16 Martin Jambor * ipa-prop.h (ipa_node_params): Convert lattices to a vector, adjust initializers in the contructor. (ipa_node_params::~ipa_node_params): Release lattices as a vector. * ipa-cp.h: New file. * ipa-cp.cc: Include sreal.h and ipa-cp.h. (ipcp_value_source): Move to ipa-cp.h. (ipcp_value_base): Likewise. (ipcp_value): Likewise. (ipcp_lattice): Likewise. (ipcp_agg_lattice): Likewise. (ipcp_bits_lattice): Likewise. (ipcp_vr_lattice): Likewise. (ipcp_param_lattices): Likewise. (ipa_get_parm_lattices): Remove assert latticess is non-NULL). (ipa_value_from_jfunc): Adjust a check for empty lattices. (ipa_context_from_jfunc): Likewise. (ipa_agg_value_from_jfunc): Likewise. (merge_agg_lats_step): Do not memset new aggregate lattices to zero. (ipcp_propagate_stage): Allocate lattices in a vector as opposed to just in contiguous memory. (ipcp_store_vr_results): Adjust a check for empty lattices. * auto-profile.cc: Include sreal.h and ipa-cp.h. * cgraph.cc: Likewise. * cgraphclones.cc: Likewise. * cgraphunit.cc: Likewise. * config/aarch64/aarch64.cc: Likewise. * config/i386/i386-builtins.cc: Likewise. * config/i386/i386-expand.cc: Likewise. * config/i386/i386-features.cc: Likewise. * config/i386/i386-options.cc: Likewise. * config/i386/i386.cc: Likewise. * config/rs6000/rs6000.cc: Likewise. * config/s390/s390.cc: Likewise. * gengtype.cc (open_base_files): Added sreal.h and ipa-cp.h to the files to be included in gtype-desc.cc. * gimple-range-fold.cc: Include sreal.h and ipa-cp.h. * ipa-devirt.cc: Likewise. * ipa-fnsummary.cc: Likewise. * ipa-icf.cc: Likewise. * ipa-inline-analysis.cc: Likewise. * ipa-inline-transform.cc: Likewise. * ipa-inline.cc: Include ipa-cp.h, move inclusion of sreal.h higher. * ipa-modref.cc: Include sreal.h and ipa-cp.h. * ipa-param-manipulation.cc: Likewise. * ipa-predicate.cc: Likewise. * ipa-profile.cc: Likewise. * ipa-prop.cc: Likewise. (ipa_node_params_t::duplicate): Assert new lattices remain empty instead of setting them to NULL. * ipa-pure-const.cc: Include sreal.h and ipa-cp.h.
