[llvm-commits] Fwd: Patch for review: Speeding up ScheduleDAG computations

[Roman Levenstein]

Hi Evan,

2008/3/4, Evan Cheng <evan.cheng at apple.com>:
>
>  On Mar 4, 2008, at 3:44 AM, Roman Levenstein wrote:
>
>  > Hi,
>  >
>  > 2008/3/3, Dan Gohman <gohman at apple.com>:
>  >>
>  >> On Mar 3, 2008, at 1:31 PM, Chris Lattner wrote:
>  >>
>  >>> On Mar 3, 2008, at 1:26 PM, Evan Cheng wrote:
>  >>>> Thanks.
>  >>>>
>  >>>> Using std::set<> makes me queasy. :-) Too bad it doesn't seem
>  >>>> possible  to use SmallPtrSet (elements are not in sorted order).
>  >>>
>  >>> I'm sad to say that std::set is probably the best thing we have
>  >>> right
>  >>> now that provides priority queue + removal from the middle support.
>  >>>
>  >>
>  >> There's make_heap/push_heap/etc. in <algorithm> that let a
>  >> plain std::vector (or a SmallVector I guess) be used as a heap.
>  >
>  > Yes, this is possible but produces much more overhead than std::set on
>  > my tests. BTW, this approach is used in DAGISel.inc files generated by
>  > tablegen. I tried to changed it to std::set as well and ,again, it
>  > works much (25%-30%) faster  on BBs with few hundreds or thousends
>  > instructions.
>
>
> If you give me a patch, I'll test it on my end. Thanks.

Fine. I'll provide this patch tomorrow. It is not a patch for the
tablegen yet, it is just a small change of the code selector generated
by the tablegen for the X86 target.

>  > I'd like to understand better why Evan and Chris are some much against
>  > std::set? From some mails I got the impression that std::set
>  > implementation is very inefficient on Darwin or PowerPC? Is it
>  > correct? Or do you dislike that fact that std::set uses dynamic memory
>  > allocation extensively?
>
>
> Right, it's very malloc intensive. That's the main issue.

I think we may have a reasonable solution here. Remember that STL
allows us to provide our own allocators as a template parameter for
all collection classes including std::set. So, in principle we can use
a cutom pool allocator or something like that to make std::set less
memory intensive.

BTW, here you can find a nice description about writing your own stl
compatible allocator:
http://www.codeguru.com/Cpp/Cpp/cpp_mfc/stl/article.php/c4079

>  >
>  >
>  >>> Does anyone know of a better data structure to implement these
>  >>> operations?  Even if you don't volunteer to implement it, we can add
>  >>> it to the open projects page.
>  >
>  > In principle, we need something like a balanced tree. And since we
>  > want to be able to remove from the middle, I'd say that it should not
>  > be mapped to something linear like std::vector, as it is done by
>  > make_heap, since it involves quite some overhead and a lot of copying
>  > around. Finding/creating a tree implementation is not such a big deal.
>  > But again, why it would be better than std::set, that anyway seems to
>  > be implemented internally as a read-black tree?
>
>
> Hard to say without testing. It might work better for small / medium
>  sized programs if it were designed to avoid malloc for smallish queues.

See above. Custom allocator may solve our problems.

>  > To conclude: Guys, I'm still waiting for a review and approval of the
>  > two pending patches for ScheduleDAGList and ScheduleDAGRRList. Both of
>  > them use std::set approach among other things.
>
>
> I thought those were approved? We understanding std::set is probably
>  the best choice of ADT for now. Thanks!

OK. I'll commit them tomorrow.

But are you sure about the second one for the ScheduleDAGRRList?
I mean this: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080303/059078.html

It also intorduces strict ordering for sorting functions and some
other changes besides std::set. Would be nice to get a confirmation
from others that it does not break any existing  test-cases and
compiled programms still produce correct results.

- Roman