[llvm-commits] [PATCH] PR12156, SmallPtrMap

[Stepan Dyatkovskiy]

Ping.
Stepan Dyatkovskiy wrote:
> Ping
> Stepan Dyatkovskiy wrote:
>> Hi Chris. I fixed all you ask.
>>
>> About MultiMapIterator.
>> For FlatArrayMap + DenseMap couple it is very reasonable to use
>> DenseMapIterator.
>> I defined MultiImplMapIteratorsFactory template that presents iterator
>> type itself and interface for iterators construction.
>> For FlatArrayMap + DenseMap it has specialization that allows to use
>> DenseMapIterator directly keeping genericity of default MultiImplMap
>> implementation.
>>
>> Just for clarity...
>> Code below:
>> typedef MultiImplMapIterator<SmallMapTy, BigMapTy> iterator;
>> is replaced with these ones:
>> typedef MultiImplIteratorsFactory<SmallMapTy, BigMapTy> ItFactory;
>> typedef typename ItFactory::iterator iterator;
>> Iterator construction that was used in previous patch:
>> return const_iterator(SmallMap.find(K))
>> is replaced with:
>> return ItFactory::const_it(SmallMap, SmallMap.find(K));
>> and so on.
>> This approach allows us to optimize iterators for some special cases and
>> to keep default behaviour for all others.
>>
>> So, please, find reworked patch in attachment for review.
>>
>> -Stepan.
>>
>> Chris Lattner wrote:
>>>
>>> On Mar 30, 2012, at 12:34 AM, Stepan Dyatkovskiy wrote:
>>>
>>>> Hi all. I reworked my patch again.
>>>> In MultiImplMap class I added "bool DenseMapCompatible" template
>>>> parameter that makes it DenseMap compatible. For DenseMapCompatible =
>>>> true user should present map implementations with DenseMap specific
>>>> methods, though.
>>>> SmallMap became special case of MultiImplMap that uses FlatArrayMap
>>>> for small mode, DenseMap for big mode and that set DenseMapCompatible
>>>> = true.
>>>
>>> Thank you for working on this Stepan! This looks like great progress,
>>> though I have a strong concern about the design (stemming from the
>>> iterator, see the end). Here are some random comments:
>>>
>>>
>>> Please include a patch that updates the data structures section of
>>> docs/ProgrammerManual.html.
>>>
>>>
>>> +//===- llvm/ADT/FlatArrayMap.h - 'Normally small' pointer set ----*-
>>> C++ -*-==//
>>> ...
>>> +// After maximum number of elements is reached, map declines any
>>> farther attempts
>>>
>>> "further attempts". Also, please fit to 80 columns.
>>>
>>>
>>>
>>> Please add a doxygen /// comment to the FlatArrayMap class definition.
>>>
>>>
>>> Out of curiosity, why use:
>>> + template<typename KeyTy, typename MappedTy, unsigned N>
>>> ...
>>> + enum { ArraySize = N };
>>>
>>> instead of just renaming 'N' to 'ArraySize'?
>>>
>>> ...
>>> + const_iterator begin() const {
>>> + return const_iterator(Array); }
>>>
>>> Please put } on the next line.
>>>
>>>
>>> + void clear() {
>>> + // Call destructor for each item and forget them.
>>> + for (unsigned i = 0; i< NumElements; ++i) {
>>>
>>> This loop will reload NumElements every time through the loop if the
>>> dtors are non-trivial, please cache it in a local variable.
>>>
>>> +//===- llvm/ADT/SmallMap.h - 'Normally small' pointer set ----*- C++
>>> -*-===//
>>>
>>> Please fill this out to exactly 80 columns.
>>>
>>> +++ include/llvm/ADT/MultiImplMap.h (revision 0)
>>>
>>> + void moveAllToBigMap() {
>>> + BigMap.insert(SmallMap.begin(), SmallMap.end());
>>> + SmallMap.clear();
>>> + }
>>>
>>> This should just be inlined into its single caller. If you really want
>>> it out of line, please indent by 2, not 4.
>>>
>>>
>>> + MultiImplMap(const self& other) {
>>> + UseSmall = other.UseSmall;
>>> + if (UseSmall)
>>> + SmallMap = other.SmallMap;
>>> + else
>>> + BigMap = other.BigMap;
>>> + }
>>>
>>> Shouldn't this check the size of "other" and use SmallMap if it can?
>>> If "other" started grew to "large" size, then had most of its elements
>>> removed (so that it could fit in "small") then was copied, it would
>>> make sense to start the copy out small.
>>>
>>>
>>> + void clear() {
>>> + if (UseSmall)
>>> + SmallMap.clear();
>>> + else
>>> + BigMap.clear();
>>> + }
>>>
>>> In contrast to the copy ctor, this should stay the way it is. Clearing
>>> a densemap doesn't free its buckets. Please add a comment to this
>>> effect so that it is clearly intentional.
>>>
>>>
>>> + void swap(MultiImplMap& rhs) {
>>> +}
>>>
>>> Why not just define this as:
>>>
>>> SmallMap.swap(rhs.SmallMap);
>>> BigMap.swap(rhs.BitMap);
>>>
>>> with no cases in it?
>>>
>>>
>>> + self& operator=(const self& other) {
>>>
>>> Similar to the 'clear' method, switching back to small doesn't help
>>> you. If 'this' is already big, stay big.
>>>
>>>
>>>
>>> + template<class SmallMapTy, class BigMapTy, bool IsConst>
>>> + class MultiImplMapIterator {
>>>
>>> I'm not a big fan of this class: it is going to be really inefficient
>>> to copy around and deal with, particularly because so many of the
>>> MultiImplMap methods are defined in terms of it. Can't we do something
>>> trickier if we drop the genericity of the underlying Array/Map
>>> classes? A densemap is just an array of buckets, exactly like the
>>> small case. If SmallMap took advantage of that, the iterator would be
>>> *much much* more efficient.
>>>
>>> -Chris
>>
>