Re: [PATCH 1/2] hash-table.h: support non-zero empty values in empty_slow (v2)
On Tue, 2020-01-14 at 11:02 +0100, Jakub Jelinek wrote: > On Mon, Jan 13, 2020 at 11:51:53PM -0500, David Malcolm wrote: > > > * cfg.s correctly has a call to memset (even with no > > > optimization) > > > for > > > hash_table::empty_slow > > I have intestigated unpatched cc1plus and in the (usually inlined) > alloc_entries I see in all the places the xcalloc or > ggc_internal_cleared_alloc > followed by either a loop storing some zeros or a memset, which is > something > I'd hope the patch also improves. It does: I looked at tree-ssa.s, at the alloc_entries code generated for: static hash_map > *edge_var_maps; In an unoptimized build: Before the patch I see the loop in the .s calling mark_empty after the calls to data_alloc and ggc_cleared_vec_alloc; After the patch there's no longer that loop in the generated .s With -O2 it's harder to follow as the ctor gets inlined into redirect_edge_var_map_add, but: Before the patch there's a loop that's zeroing the keys after the call to ggc_internal_cleared_alloc After the patch that loop isn't present. > As for graphite.c, sure, empty_zero_p = false is a fallback value, > the > question was not directly related to this patch, but rather wondering > how it > can work at all. > > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu (in > > conjuction with the analyzer patch kit, which it fixes) > > > > OK for master? > > Yes to both patches, thanks. Thanks. I rebased and did another bootstrap®ression test with just patch 1 to be sure. I've pushed it to master as 7ca50de02cf12c759f4f8daf60913704782b7d45. I've rebased and squashed the analyzer patch kit and squashed patch 2 into it, and am re-testing it now. Dave
Re: [PATCH 1/2] hash-table.h: support non-zero empty values in empty_slow (v2)
On Mon, Jan 13, 2020 at 11:51:53PM -0500, David Malcolm wrote: > > * cfg.s correctly has a call to memset (even with no optimization) > > for > > hash_table::empty_slow I have intestigated unpatched cc1plus and in the (usually inlined) alloc_entries I see in all the places the xcalloc or ggc_internal_cleared_alloc followed by either a loop storing some zeros or a memset, which is something I'd hope the patch also improves. As for graphite.c, sure, empty_zero_p = false is a fallback value, the question was not directly related to this patch, but rather wondering how it can work at all. > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu (in > conjuction with the analyzer patch kit, which it fixes) > > OK for master? Yes to both patches, thanks. > gcc/cp/ChangeLog: Jakub Jelinek > * cp-gimplify.c (source_location_table_entry_hash::empty_zero_p): > New static constant. > * cp-tree.h (named_decl_hash::empty_zero_p): Likewise. > (struct named_label_hash::empty_zero_p): Likewise. > * decl2.c (mangled_decl_hash::empty_zero_p): Likewise. > > gcc/ChangeLog: Jakub Jelinek , > David Malcolm > * attribs.c (excl_hash_traits::empty_zero_p): New static constant. > * gcov.c (function_start_pair_hash::empty_zero_p): Likewise. > * graphite.c (struct sese_scev_hash::empty_zero_p): Likewise. > * hash-map-tests.c (selftest::test_nonzero_empty_key): New selftest. > (selftest::hash_map_tests_c_tests): Call it. > * hash-map-traits.h (simple_hashmap_traits::empty_zero_p): > New static constant, using the value of = H::empty_zero_p. > (unbounded_hashmap_traits::empty_zero_p): Likewise, using the value > from default_hash_traits . > * hash-map.h (hash_map::empty_zero_p): Likewise, using the value > from Traits. > * hash-set-tests.c (value_hash_traits::empty_zero_p): Likewise. > * hash-table.h (hash_table::alloc_entries): Guard the loop of > calls to mark_empty with !Descriptor::empty_zero_p. > (hash_table::empty_slow): Conditionalize the memset call with a > check that Descriptor::empty_zero_p; otherwise, loop through the > entries calling mark_empty on them. > * hash-traits.h (int_hash::empty_zero_p): New static constant. > (pointer_hash::empty_zero_p): Likewise. > (pair_hash::empty_zero_p): Likewise. > * ipa-devirt.c (default_hash_traits ::empty_zero_p): > Likewise. > * ipa-prop.c (ipa_bit_ggc_hash_traits::empty_zero_p): Likewise. > (ipa_vr_ggc_hash_traits::empty_zero_p): Likewise. > * profile.c (location_triplet_hash::empty_zero_p): Likewise. > * sanopt.c (sanopt_tree_triplet_hash::empty_zero_p): Likewise. > (sanopt_tree_couple_hash::empty_zero_p): Likewise. > * tree-hasher.h (int_tree_hasher::empty_zero_p): Likewise. > * tree-ssa-sccvn.c (vn_ssa_aux_hasher::empty_zero_p): Likewise. > * tree-vect-slp.c (bst_traits::empty_zero_p): Likewise. > * tree-vectorizer.h > (default_hash_traits::empty_zero_p): > Likewise. Jakub
[PATCH 1/2] hash-table.h: support non-zero empty values in empty_slow (v2)
On Mon, 2020-01-13 at 21:58 -0500, David Malcolm wrote: > On Tue, 2020-01-14 at 00:55 +0100, Jakub Jelinek wrote: > > On Mon, Jan 13, 2020 at 06:42:06PM -0500, David Malcolm wrote: > > > Thanks. Does it have warnings, though? > > > > > > My attempt was similar, but ran into warnings from -Wclass- > > > memaccess in > > > four places, like this: > > > > > > ../../src/gcc/hash-map-traits.h:102:12: warning: ‘void* > > > memset(void*, > > > int, size_t)’ clearing an object of type ‘struct > > > hash_map > > std::pair >::hash_entry’ with no trivial > > > copy- > > > assignment; use assignment or value-initialization instead [- > > > Wclass- > > > memaccess] > > >102 | memset (entry, 0, sizeof (T) * count); > > >| ~~~^~ > > > > > > where the types in question are: > > > > > > (1) > > >struct hash_map > > > > ::hash_entry > > >../../src/gcc/tree-data-ref.c:844:17: required from here > > > > I don't understand how there could be new warnings. > > The patch doesn't add any new memsets, all it does is if (0) code > > in > > alloc_entries for certain traits and in empty_slow stops using > > memset > > for some traits and uses mark_empty loop there instead. > > Your patch didn't add new memsets; mine did - clearly I misspoke when > saying they were similar, mine had a somewhat different approach. > > Sorry for the noise, and thanks for your patch. > > I've extended your patch to cover the various hash traits in the > analyzer and it passes the analyzer's tests. I also added in the > selftests from my older patch. > > I examined the generated code, and it seems correct: > > * cfg.s correctly has a call to memset (even with no optimization) > for > hash_table::empty_slow > > * the hash_map example with nonzero empty for the analyzer: > analyzer/program-state.s: sm_state_map::remap_svalue_ids: hash_map > map_t; correctly shows a loop of calls to > mark_empty > > * a hash_map example with zero empty: tree-ssa.s edge_var_maps > correctly has a memset in the empty_slow, even with no optimization. > > ...which is promising. > > > For the graphite case, I wondered what happens if both is_empty and > is_deleted are true; looking at hash-table.h, sometimes one is > checked > first, sometimes the other, but I don't think it matters for this > case:; you have empty_zero_p = false,so it uses mark_empty, rather > than > trying to memset, which is correct - empty_zero_p being true can be > thought of as a "it is safe to optimize this hash_table/hash_map by > treating empty slots as all zero", if you will. > > > I'm trying a bootstrap build and full regression suite now, for all > languages (with graphite enabled, I believe). Will post the patches > if > it succeeds... > > > This was non-bootstrapped build, but I didn't see new warnings in > > there, > > and for tree-data-ref.c which you've mentioned I've tried to > > compile > > with installed trunk compiler and didn't get any warnings either. > > > >Jakub > > Thanks! Sorry again about getting this wrong. > > Dave Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu (in conjuction with the analyzer patch kit, which it fixes) OK for master? gcc/cp/ChangeLog: Jakub Jelinek * cp-gimplify.c (source_location_table_entry_hash::empty_zero_p): New static constant. * cp-tree.h (named_decl_hash::empty_zero_p): Likewise. (struct named_label_hash::empty_zero_p): Likewise. * decl2.c (mangled_decl_hash::empty_zero_p): Likewise. gcc/ChangeLog: Jakub Jelinek , David Malcolm * attribs.c (excl_hash_traits::empty_zero_p): New static constant. * gcov.c (function_start_pair_hash::empty_zero_p): Likewise. * graphite.c (struct sese_scev_hash::empty_zero_p): Likewise. * hash-map-tests.c (selftest::test_nonzero_empty_key): New selftest. (selftest::hash_map_tests_c_tests): Call it. * hash-map-traits.h (simple_hashmap_traits::empty_zero_p): New static constant, using the value of = H::empty_zero_p. (unbounded_hashmap_traits::empty_zero_p): Likewise, using the value from default_hash_traits . * hash-map.h (hash_map::empty_zero_p): Likewise, using the value from Traits. * hash-set-tests.c (value_hash_traits::empty_zero_p): Likewise. * hash-table.h (hash_table::alloc_entries): Guard the loop of calls to mark_empty with !Descriptor::empty_zero_p. (hash_table::empty_slow): Conditionalize the memset call with a check that Descriptor::empty_zero_p; otherwise, loop through the entries calling mark_empty on them. * hash-traits.h (int_hash::empty_zero_p): New static constant. (pointer_hash::empty_zero_p): Likewise. (pair_hash::empty_zero_p): Likewise. * ipa-devirt.c (default_hash_traits ::empty_zero_p): Likewise. * ipa-prop.c (ipa_bit_ggc_hash_traits::empty_zero_p): L
