On Wed, 2019-12-11 at 17:00 +0100, Jakub Jelinek wrote: > On Wed, Dec 11, 2019 at 10:44:58AM -0500, David Malcolm wrote: > > Is it OK for a hash_map key to have a "empty" value that isn't > > all-zeroes (and thus the same for a hash_table entry)? > > > > Is the following patch OK for trunk? > > I'd say that it is important to analyze the generated code for the > common > case where empty elt is all zeros (and perhaps POD only). > Perhaps -ftree-loop-distribute-patterns can handle it in most cases? > Perhaps only when built in C++14 or later mode, that would still > affect > bootstrapped compilers. Like, could we conditionally constexpr > evaluate > what mark_empty does and determine at compile time if it is all zeros > or > not or something similar? > > Jakub
I did a release build with gcc 9 at -O2, and examined the generated code for cfg.o's class hash_table<bb_copy_hasher>'s empty_slow (which has such an all zeroes empty element). The mark_empty loop there was converted to a memset (without needing -ftree-loop-distribute-patterns), which is promising. However, that only covers hash_table. Our hash_map is built on top of hash_table, where the entries in the hash_map are key/value pairs. A memset to 0 of all of the entries in such a hash_table is not the same as a per-entry mark_empty, as the latter only touches the m_key part of each entry, not the m_value part. It doesn't matter that we don't preserve the m_value for my POD value cases, but it means the optimizer could never replace a loop of mark_empty of m_key of 0 with a memset to 0 of all entries, since it doesn't "know" that m_value can be safely zeroed also. Is that an issue? Is it necessarily better to memset the whole of the buffer, rather than zeroing just the keys? Hacking out hash_map::empty shows that it's used in 6 places in the code currently. [Jeff approved the analyzer, but I realise that this issue is still blocking it; bother] Dave
