On Thu, 21 Mar 2019, Jakub Jelinek wrote: > 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.
Oh. I was mostly looking at the cases you add, find_decls_types_r is indeed special. > 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. Yeah, I think there's no point to add it again. Or is there a case I am missing - Honza? > 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? Not sure if that's needed, if you leave find_decls_types_r alone then always doing the check should work, no? Richard.
