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

[David Greene]

On Friday 29 February 2008 11:27, Roman Levenstein wrote:

> Now,  a new problem I have descovered:
> While improving my patch, I also looked at the LatencyPriorityQueue
> implementation and  the RegReductionPriorityQueue implementation. Both
> are using priority queues and have an extremely inefficient remove()
> method, that consumes 90% of the compilation time on very big BBs. So,
> I changed the data structure from the priority queue to the ordered
> std::set. Obviously, the max element is basically the last element of
> the set. This change produces a 3x speedup of compilation time on huge
> basic blocks. But there is a problem (or a bug!):

The priority queue implementation is simply broken.  The heap properties
are not properly maintained.  I proposed a patch to fix this some time ago
but it is still in limbo.  It also cleans up the remove() infefficiency but 
trades it off for time to recompute the heap.  I've found it to be close to a
wash, slower on some codes and faster on others.

Evan, what do I need to do to get that patch into acceptable condition?

> the comparison operator
>  td_ls_rr_sort::operator()(const SUnit* left, const SUnit* right) (see
> ScheduleDAGRRList.cpp)
> seems to be implemented incorrectly (see the code below).

Heh.  Yep, this looks familiar.

> Namely, I discovered by debugging two specific values L and R, where
> L!=R, so that this operator returns FALSE (by return statement at the
> very end of the method) for both pair (L,R) and (R,L). That means that
> L >= R and R >=L and L!=R at the same time, which cannot be.  This
> incorrect comparison operator breaks all set operations based on the
> ordering of elements, e.g. find() and insert(). Actually, I'm
> wondering why it didn't break the priority queue operations before.

Configure with --enable-expensive-checks and you'll probably get an assert.

> The same operator for  bu_ls_rr_sort has a very long disclaimer
> related to a similar issue:
>   // There used to be a special tie breaker here that looked for
>   // two-address instructions and preferred the instruction with a
>   // def&use operand.  The special case triggered diagnostics when
>   // _GLIBCXX_DEBUG was enabled because it broke the strict weak
>   // ordering that priority_queue requires. It didn't help much anyway
>   // because AddPseudoTwoAddrDeps already covers many of the cases
>   // where it would have applied.  In addition, it's counter-intuitive
>   // that a tie breaker would be the first thing attempted.  There's a
>   // "real" tie breaker below that is the operation of last resort.
>   // The fact that the "special tie breaker" would trigger when there
>   // wasn't otherwise a tie is what broke the strict weak ordering
>   // constraint.

Yep, I believe Dan Gohman fixed this one.

> So, there seems to be a problem. But I don't have a solution for it. I
> need you help to understand how to fix it.

It's possible the compare bug was not caught earlier because 
std::poriority_queue doesn't check it with _GLIBCXX_DEBUG turned
on.  I don't know that for a fact, though.

I would think that eliminating the special tie breaker might be a solution,
similarly to what was done in bu_ls_rr_sort.

I don't think this change will fix the issues with the priority queue because
it's a problem with how the scheduling algorithm works.  The algorithm updates
priorities but doesn't call make_heap to re-sort the nodes.  That call has to
happen whenever priorities are updated.  My patch fixes that problem.

So I think we need both fixes.

                                                -Dave