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

[Roman Levenstein]

Hi David, Hi Evan,

2008/3/1, Evan Cheng <evan.cheng at apple.com>:
>
>  On Feb 29, 2008, at 12:29 PM, David Greene wrote:
>
>  > 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?
>
>
> Please resubmit it again. I'll evaluate the compile time penalty today.

David, I guess you mean this patch?
http://www.nabble.com/RFC:-GLIBCXX_DEBUG-ScheduleDAG-Patch-td14382214.html

Looking at it, I'd assume it is not extremely efficient since you
still use the priority queue and rebuild it, when you remove elements.

I'm pretty much sure that using std::set or any other efficient sorted
set implementation (binary tree,etc) would perform better, if we need
to delete elements from the middle of the heap and update priorities
(which can be done by means of remove,change,insert). My preliminary
tests done using std::set instead of priority queues for td-sched
indicate a 3x performance boost.

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

Good that you confirm it.

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

Could be.

>  > I would think that eliminating the special tie breaker might be a
>  > solution,
>  > similarly to what was done in bu_ls_rr_sort.
>
>
> There is something I am not grasping. I would think the problem can
>  still occur even if the special tie breaker is removed.  Take any two
>  nodes A, B, suppose all of the conditions evaluates to false the
>  function will returns false regardless if it's comparing A <= B or B
>  <= A, right? I would think the right is just to change the default from
>  return false;
>  to
>  return left->NodeNum < right->NodeNum;
>
>  This forces a strict ordering between two nodes, no?

Agreed, this should introduce a strict ordering and fix the problem.

So, we need your fix to update the priorities, but I don't think that
your data-structures are efficient. So, I'd like to ask you to wait
until Monday, until I test my patch including your priority update
changes. This would avoid any commit conflicts. Based on the
performance test, we'll decide which approach should be committet. Is
it OK with you? I'd be glad to submit the patch immediately, but I
forgot my development machine in the office ;-(

Regards,
  Roman