[llvm-commits] SmallMap class

[Stepan Dyatkovskiy]

Hi Chris,

The post is here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20120402/140431.html

The patch itself is here:
http://lists.cs.uiuc.edu/pipermail/llvm-commits/attachments/20120406/ffdbbfcf/attachment.bin

-Stepan.

Chris Lattner wrote:
> Hi Stephan,
>
> Where is the most recent version of the patch?
>
> -Chris
>
> On Apr 19, 2012, at 12:45 AM, Stepan Dyatkovskiy wrote:
>
>> Hello guys. Are you still interested in SmallMap class?
>> -Stepan.
>> Stepan Dyatkovskiy wrote:
>>> 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
>>>>>
>>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits