| Message ID | ri6o7clzimt.fsf@virgil.suse.cz |
|---|---|
| State | New |
| Headers | show |
| Series | ipa: call destructors on lattices before freeing them (PR 113476) | expand |
> Hi, > > In PR 113476 we have discovered that ipcp_param_lattices is no longer > a POD and should be destructed. This patch does that, calling > destructor on each element of the array containing them when the > corresponding summary of a node is freed. An alternative would be to > change the XCNEWVEC-and-placement-new to initializations in > constructors of all things in ipcp_param_lattices and then simply use > normal operators new and delete. I am not sure, the initialization > through XCNEWVEC may be a bit more efficient although that is probably > not a big concern. In the end, I opted for a simpler solution for > stage 4. > > I have verified that valgrind no longer reports lost memory blocks > allocated within ipcp_vr_lattice::meet_with_1 on the preprocessed source > (dwarf2out.i) attached to Bugzilla. The patch also passes bootstrap and > LTO bootstrap and testing on x86_64-linux. > > OK for master? > > Thanks, > > Martin > > > gcc/ChangeLog: > > 2024-02-09 Martin Jambor <mjambor@suse.cz> > > PR tree-optimization/113476 > * ipa-prop.h (ipa_node_params::~ipa_node_params): Moved... > * ipa-cp.cc (ipa_node_params::~ipa_node_params): ...here. Added > destruction of lattices. OK. So you do not use vectors (which would also handle the destruction) basically to save space needed to keep the size of the vector since that is known from the parameter count? Honza
On Mon, Feb 12 2024, Jan Hubicka wrote: >> Hi, >> >> In PR 113476 we have discovered that ipcp_param_lattices is no longer >> a POD and should be destructed. This patch does that, calling >> destructor on each element of the array containing them when the >> corresponding summary of a node is freed. An alternative would be to >> change the XCNEWVEC-and-placement-new to initializations in >> constructors of all things in ipcp_param_lattices and then simply use >> normal operators new and delete. I am not sure, the initialization >> through XCNEWVEC may be a bit more efficient although that is probably >> not a big concern. In the end, I opted for a simpler solution for >> stage 4. >> >> I have verified that valgrind no longer reports lost memory blocks >> allocated within ipcp_vr_lattice::meet_with_1 on the preprocessed source >> (dwarf2out.i) attached to Bugzilla. The patch also passes bootstrap and >> LTO bootstrap and testing on x86_64-linux. >> >> OK for master? >> >> Thanks, >> >> Martin >> >> >> gcc/ChangeLog: >> >> 2024-02-09 Martin Jambor <mjambor@suse.cz> >> >> PR tree-optimization/113476 >> * ipa-prop.h (ipa_node_params::~ipa_node_params): Moved... >> * ipa-cp.cc (ipa_node_params::~ipa_node_params): ...here. Added >> destruction of lattices. > > OK. > So you do not use vectors (which would also handle the destruction) > basically to save space needed to keep the > size of the vector since that is known from the parameter count? > 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. Martin
> 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. Honza > > Martin
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. Thanks, Martin
Hi, On Mon, Feb 12 2024, Jan Hubicka wrote: >> Hi, >> >> In PR 113476 we have discovered that ipcp_param_lattices is no longer >> a POD and should be destructed. This patch does that, calling >> destructor on each element of the array containing them when the >> corresponding summary of a node is freed. An alternative would be to >> change the XCNEWVEC-and-placement-new to initializations in >> constructors of all things in ipcp_param_lattices and then simply use >> normal operators new and delete. I am not sure, the initialization >> through XCNEWVEC may be a bit more efficient although that is probably >> not a big concern. In the end, I opted for a simpler solution for >> stage 4. >> >> I have verified that valgrind no longer reports lost memory blocks >> allocated within ipcp_vr_lattice::meet_with_1 on the preprocessed source >> (dwarf2out.i) attached to Bugzilla. The patch also passes bootstrap and >> LTO bootstrap and testing on x86_64-linux. >> >> OK for master? >> >> Thanks, >> >> Martin >> >> >> gcc/ChangeLog: >> >> 2024-02-09 Martin Jambor <mjambor@suse.cz> >> >> PR tree-optimization/113476 >> * ipa-prop.h (ipa_node_params::~ipa_node_params): Moved... >> * ipa-cp.cc (ipa_node_params::~ipa_node_params): ...here. Added >> destruction of lattices. > > OK. > So you do not use vectors (which would also handle the destruction) > basically to save space needed to keep the > size of the vector since that is known from the parameter count? > OK, so when I started looking at converting lattices to vector, it immediately became clear why it is an array. The type of the element of the array (ipcp_param_lattices and all it contains) is only forward declared in ipa-prop.h where ipa_node_params is defined which can therefore just contain a pointer. The actual definition of ipcp_param_lattices is then done only in ipa-cp.c. Converting the array to a vector would means moving ipcp_param_lattices together with ipcp_lattice, ipcp_value, ipcp_value_base, ipcp_agg_lattice, ipcp_bits_lattice, ipcp_vr_lattice from ipa-cp.c to ipa-prop.h. Or an ipa-cp.h which ipa-prop.h would require/include. But perhaps that is the proper C++ thing to do :-/ Martin
diff --git a/gcc/ipa-cp.cc b/gcc/ipa-cp.cc index e85477df32d..9864ff052de 100644 --- a/gcc/ipa-cp.cc +++ b/gcc/ipa-cp.cc @@ -399,6 +399,23 @@ public: bool virt_call; }; +/* Destructor of node function summary, placed here because it mainly must + destruct value range lattices not known outside of this source file. */ + +ipa_node_params::~ipa_node_params () +{ + if (lattices) + { + int count = ipa_get_param_count (this); + for (int i = 0; i < count; i++) + lattices[i].~ipcp_param_lattices (); + free (lattices); + } + vec_free (descriptors); + known_csts.release (); + known_contexts.release (); +} + /* Allocation pools for values and their sources in ipa-cp. */ object_allocator<ipcp_value<tree> > ipcp_cst_values_pool diff --git a/gcc/ipa-prop.h b/gcc/ipa-prop.h index 9c78dc9f486..fe401640824 100644 --- a/gcc/ipa-prop.h +++ b/gcc/ipa-prop.h @@ -670,15 +670,6 @@ ipa_node_params::ipa_node_params () { } -inline -ipa_node_params::~ipa_node_params () -{ - free (lattices); - vec_free (descriptors); - known_csts.release (); - known_contexts.release (); -} - /* Intermediate information that we get from alias analysis about a particular parameter in a particular basic_block. When a parameter or the memory it references is marked modified, we use that information in all dominated