[llvm-commits] Patch: SparseMultiSet

[Andrew Trick]

On Jan 17, 2013, at 2:34 PM, Michael Ilseman <milseman at apple.com> wrote:

> New patch. I have the iterators save an index into the sparse array so they can re-issue a find when they're end-decremented. This means that they're no longer bi-modal. I fixed the other suggestions and added more test coverage.

Changes to ScheduleDAGInstrs.cpp look good.

Nit:
+    } else {
...
+      if (SU->isCall) {

++    } else if (SU->isCall) {

SparseMultiSet will be really nice to have. To be honest, we're going from something that was constant time for the frequent operation of clearing the def/use lists to linear time. But in practice it will work great.

Your implementation is very nice and thorough. Here are questions from my first read-through. Just cases that I either didn't understand or could be slightly simplified.

In particular, some code implies that an internal index can be outside the Dense set. I'm not sure if that's actually expected or we're just being cautious about checking for invalid Nodes/iterators.

+  bool isHead(const SMSNode &D) const {
+    assert(D.isValid() && "Invalid node for head");
+    return D.Prev < Dense.size() && Dense[D.Prev].isTail();
+  }

How can D.Prev be >= Dense.size?

+    return (&Dense[0] + N.Prev) == &N;

Why not the obvious?

++    return &Dense[N.Prev] == &N;

+      if (Idx > SMS->Dense.size()) {

Why not...

++     if (Idx == INVALID)

...shouldn't we catch out-of-bounds Idx elsewhere?

+    bool operator==(const iterator_base &RHS) const {
+      // end compares equal
+      return SMS == RHS.SMS && Idx == RHS.Idx;

Shouldn't we verify that either SparseIdx are the same or INVALID?

+  iterator end() { return iterator(this, SMSNode::INVALID, 0); }

Why not..

+  iterator end() { return iterator(this, SMSNode::INVALID, INVALID); }

Wouldn't that be easier to verify and avoid decrementing into a valid iterator?

+      // Singleton is already unlinked
+      return iterator(this, N.Next, ValIndexOf(N.Data));

Would this be more clear?

++      return iterator(this, INVALID, ValIndexOf(N.Data));

+      // If we're going off the end, then set our mode to be at the end and keep
+      // the Idx as a previous index.
+      if (Next() < SMS->Dense.size())
+        Idx = Next();
+      else
+        Idx = SparseMultiSet::SMSNode::INVALID;

I'm not sure what that means and why not just:

++ Idx = Next()


-Andy

> On Jan 17, 2013, at 10:45 AM, Michael Ilseman <milseman at apple.com> wrote:
> 
>> 
>> On Jan 17, 2013, at 10:15 AM, Jakob Stoklund Olesen <stoklund at 2pi.dk> wrote:
>> 
>>> 
>>> On Jan 16, 2013, at 10:39 AM, Michael Ilseman <milseman at apple.com> wrote:
>>> 
>>>> Attached is a patch introducing a new data structure, the SparseMultiSet, and changes to the MI scheduler to use it.
>>>> 
>>>> A SparseMultiSet adds multiset behavior to SparseSet, while retaining SparseSet's desirable properties. Essentially, SparseMultiSet provides multiset behavior by storing its dense data in doubly linked lists that are inlined into the dense vector. This allows it to provide good data locality as well as vector-like constant-time clear() and fast constant time find(), insert(), and erase(). It also allows SparseMultiSet to have a builtin recycler rather than keeping SparseSet's behavior of always swapping upon removal, which allows it to preserve more iterators. It's often a better alternative to a SparseSet of a growable container or vector-of-vector.
>>> 
>>> +    return (Dense.begin() + N.Prev) == &N;
>>> 
>>> AFAICT, this assumes that begin() is a pointer, but it could be some iterator class.
>>> 
>> 
>> True, I'll do (&Dense[0] + N.Prev) == &N; This keeps the requirement that it be laid out in contiguous memory, but removes the dependence on pointer-iterators.
>> 
>>> +    const iterator_base &operator=(const iterator_base &RHS) {
>>> +      Dense = RHS.Dense;
>>> 
>>> Dense is a reference, so this assigns the whole array. Please make sure you have a test case that catches this.
>>> 
>> 
>> Good catch, I'll change to a pointer rather than a reference.
>> 
>>> +    /// Increment and decrement operators
>>> +    iterator_base &operator--() { // predecrement - Back up
>>> +      assert(isValid() && "Decrementing an invalid iterator");
>>> +
>>> +      // If we're at the end, then interpret our index as a previous index
>>> +      if (IsEnd)
>>> +        IsEnd = false;
>>> 
>>> How do you know Idx is still valid here? Please add a test that decrements end() after erasing elements.
>>> 
>> 
>> You're right, to ensure that end iterators are still valid post tail erasure, I'll have to re-issue a find on end decrement.
>> 
>>> +  std::pair<iterator,iterator> equal_range(const KeyT &K) {
>>> +    iterator I = find(K);
>>> +    if (I == end())
>>> +      return make_pair(I, I);
>>> +
>>> +    iterator B = I--;
>>> +    return make_pair(B, ++I);
>>> +  }
>>> 
>>> This looks very odd. I don't think the iterator class should expose to clients that it is a circular linked list. It would be better is --I on find() were an assertion. Use private methods to access the linked list.
>>> 
>> 
>> Will do.
>> 
>>> It looks like you need better test coverage for all the corner cases.
>>> 
>> 
>> Will do.
>> 
>>> /jakob
>>> 
>> 
>> Thanks!
>> _______________________________________________
>> 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