[PATCH] D35148: Use DenseMap instead std::map for GVSummaryMapTy.

[David Blaikie via llvm-commits]

On Mon, Jul 10, 2017 at 6:27 AM Teresa Johnson via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> On Sun, Jul 9, 2017 at 7:35 PM, Mehdi AMINI via Phabricator <
> reviews at reviews.llvm.org> wrote:
>
>> mehdi_amini added a comment.
>>
>> In https://reviews.llvm.org/D35148#803251, @tejohnson wrote:
>>
>> > In https://reviews.llvm.org/D35148#803193, @mehdi_amini wrote:
>> >
>> > > When we used an std::map originally it was because we needed the
>> ordering. Have you checked that we don't iterate on any instance of this
>> map?
>> >
>> >
>> > From http://llvm.org/docs/ProgrammersManual.html#llvm-adt-densemap-h
>> it doesn't appear that iteration order is an issue for DenseMap.
>>
>>
>> As long as keys aren't pointers...
>> (But just as every hash_map right?)
>>
>
> Right.
>
>
>>
>> > StringMap on the other hand does not have a deterministic iteration
>> ordering, and I recall using a std::map somewhere instead of a StringMap
>> for that reason.
>>
>> Really? I'm pretty sure the order is deterministic, but just like any
>> hash_map (including DenseMap): the order of insertion and the hash function
>> are determining the resulting order.
>>
>
> According to the end of
> http://llvm.org/docs/ProgrammersManual.html#llvm-adt-stringmap-h the
> iteration order is non-deterministic.
>

Yeah, this sort of gets into fuzzy places. LLVM's hash-based structures
(this goes for DenseMap, StringMap, etc) are currently implemented
deterministically, but arbitrarily ordered (based on hash and insertion
order - I wonder if they change based on how many buckets they have (eg:
could you get different order if you insert some things, clear, then insert
some new things)).

It's not entirely clear whether we want to make LLVM's output stable
relative to changes to these data structures internals changing (eg:
changing hash, load factor, etc)... - I kind of like the idea of doing
that, but it does come at some costs. If we don't do that then LLVM as a
whole still probably has stable output (until someone gets annoyed with
cleaning up tests when tuning these hashing data structures - and does what
some standard library implementations have done: mix in a random number to
the hash function (either random per execution, per build, or even per
instance of the data structure)).


>
>
>> > Note the index itself still uses a std::map, for other reasons
>> (documented at the declaration). We also still use a std::map where we
>> write out the individual index files for the backends in the distributed
>> backend case. In any case from what I can tell, we typically query this
>> map, rather than iterate. The only place I see an iteration is when we are
>> computing imports for a module, so I suppose that could affect the debug
>> output ordering, but does that matter (if iteration order is even
>> non-deterministic for DenseMap, and from what I can tell from the LLVM doc
>> linked above it isn't).
>>
>> OK!
>>
>>
>> https://reviews.llvm.org/D35148
>>
>>
>>
>>
>
>
> --
> Teresa Johnson |  Software Engineer |  tejohnson at google.com |
> 408-460-2413 <(408)%20460-2413>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170710/fadead43/attachment-0001.html>