On Thu, Mar 21, 2019 at 10:11:00AM +0100, Richard Biener wrote:
> On Thu, 21 Mar 2019, Jakub Jelinek wrote:
>
> > On Thu, Mar 21, 2019 at 09:35:07AM +0100, Richard Biener wrote:
> > > LGTM - any reason you only sometimes conditionalize add_tree_to_fld_list
> > > on the fld->pset.add () result? IMHO consistency should win over
> > > micro-optimizing the case you know the type will not be in the set.
> >
> > Yeah, it was a optimization for the most common case, when
> > find_decls_types_r calls add_tree_to_fld_list (t, fld); for DECL_P as well
> > as TYPE_P, because in that case the fld->pset.add check has been done
> > already by walk_tree. If we have hundreds thousands of elts in the
> > hash_set, it could be more than a microoptimization, but if you think
> > strongly that it should be all thrown away and add_tree_to_fld_list should
> > start with if (fld->pset.add (t)) return; then I can do that too.
>
> Yes, I think that would be better.
Actually sorry, that is not going to work, because in the two
find_decls_types_r cases it is already set in the pset by walk_tree and thus
fld->pset.add (t) will return true because it is the second addition.
And just doing fld->pset.add (t) and not testing the result is undesirable
too for the other cases; in the patch there was some set of cases where I
was sure that the type isn't already in the pset (e.g. immediately after
build_variant_type_copy or build_distinct_type_copy), but in several other
cases it is unclear if it is already in or not and I think we don't want to
add it again in those cases.
Would you prefer to have say add_tree_to_fld_list that does what it does now
and has just 2 callers in find_decls_types_r and a new
maybe_add_tree_to_fld_list that would do
if (!fld->pset.add (t))
add_tree_to_fld_list (t, fld);
and change all the other add_tree_to_fld_list callers?
Jakub
