[llvm] r310189 - [ADT] Add a much simpler loop to DenseMap::clear when the types are

[David Blaikie via llvm-commits]

On Mon, Aug 7, 2017 at 11:54 AM Chandler Carruth <chandlerc at gmail.com>
wrote:

> On Mon, Aug 7, 2017 at 11:33 AM David Blaikie via llvm-commits <
> llvm-commits at lists.llvm.org> wrote:
>
>> On Sat, Aug 5, 2017 at 3:49 PM Chandler Carruth via llvm-commits <
>> llvm-commits at lists.llvm.org> wrote:
>>
>>> Author: chandlerc
>>> Date: Sat Aug  5 15:48:37 2017
>>> New Revision: 310189
>>>
>>> URL: http://llvm.org/viewvc/llvm-project?rev=310189&view=rev
>>> Log:
>>> [ADT] Add a much simpler loop to DenseMap::clear when the types are
>>> POD-like and we can just splat the empty key across memory.
>>>
>>> Sadly we can't optimize the normal loop well enough because we can't
>>> turn the conditional store into an unconditional store according to the
>>> memory model.
>>>
>>> This loop actually showed up in a profile of code that was calling clear
>>> as a serious source of time. =[
>>>
>>> Modified:
>>>     llvm/trunk/include/llvm/ADT/DenseMap.h
>>>
>>> Modified: llvm/trunk/include/llvm/ADT/DenseMap.h
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/DenseMap.h?rev=310189&r1=310188&r2=310189&view=diff
>>>
>>> ==============================================================================
>>> --- llvm/trunk/include/llvm/ADT/DenseMap.h (original)
>>> +++ llvm/trunk/include/llvm/ADT/DenseMap.h Sat Aug  5 15:48:37 2017
>>> @@ -107,17 +107,23 @@ public:
>>>      }
>>>
>>>      const KeyT EmptyKey = getEmptyKey(), TombstoneKey =
>>> getTombstoneKey();
>>> -    unsigned NumEntries = getNumEntries();
>>> -    for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E; ++P) {
>>> -      if (!KeyInfoT::isEqual(P->getFirst(), EmptyKey)) {
>>> -        if (!KeyInfoT::isEqual(P->getFirst(), TombstoneKey)) {
>>> -          P->getSecond().~ValueT();
>>> -          --NumEntries;
>>> -        }
>>> +    if (isPodLike<KeyT>::value && isPodLike<ValueT>::value) {
>>>
>>
>> What if these conditionals ^ were rolled into the general form of the
>> loop in the else block? So that the dtor wouldn't be invoked for pod value
>> types, even if the key wasn't pod - and the key would be assigned to
>> EmptyKey without an isEqual check if the key is pod even if the value isn't.
>>
>
> I was worried about assigning over that many keys unnecessarily if that
> assignment might be slow (IE, call an assignment operator).
>

Not sure I quite follow how this concern follows from my
question/suggestion - perhaps I missed a step or two, or my description was
poorly worded?

Here's what I was thinking:

... And as I write that out (I didn't actually get to any of the
modifications/suggestions) I see that if the value type is non-pod then the
key must be tested for empty/tombstone anyway, so my suggestion wouldn't
save much in that case (it'd save the empty comparison if the tombstone
comparison was true, if they key is pod).

The POD-ness is admittedly a poor proxy for this... other ideas?
>

is_trivially_assignable?


>
>
>>
>>
>>> +      // Use a simpler loop when these are trivial types.
>>> +      for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E; ++P)
>>>          P->getFirst() = EmptyKey;
>>> +    } else {
>>> +      unsigned NumEntries = getNumEntries();
>>> +      for (BucketT *P = getBuckets(), *E = getBucketsEnd(); P != E;
>>> ++P) {
>>> +        if (!KeyInfoT::isEqual(P->getFirst(), EmptyKey)) {
>>> +          if (!KeyInfoT::isEqual(P->getFirst(), TombstoneKey)) {
>>> +            P->getSecond().~ValueT();
>>> +            --NumEntries;
>>> +          }
>>> +          P->getFirst() = EmptyKey;
>>> +        }
>>>        }
>>> +      assert(NumEntries == 0 && "Node count imbalance!");
>>>      }
>>> -    assert(NumEntries == 0 && "Node count imbalance!");
>>>      setNumEntries(0);
>>>      setNumTombstones(0);
>>>    }
>>>
>>>
>>> _______________________________________________
>>> llvm-commits mailing list
>>> llvm-commits at lists.llvm.org
>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>>>
>> _______________________________________________
>> 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/20170807/3727c436/attachment-0001.html>