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

[Evan Cheng]

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.

>
>
> 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.

>
>
>>> 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.

>
>
> 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!

Evan

>
>
> - Roman
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits