[llvm-commits] Speeding up instruction selection (ScheduleDAGRRList)

[Roman Levenstein]

Hi Evan, Hi Dan,

2008/4/24, Dan Gohman <gohman at apple.com>:
> Hi Roman,
>
>  I have a few more comments for the ScheduleDAGRRList.cpp queue patch:
>
>  > -    virtual void updateNode(const SUnit *SU) {}
>  > +    virtual void updateNode(const SUnit *SU) {
>  > +      remove((SUnit *)SU);
>  > +      push((SUnit *)SU);
>  > +    }
>
>  updateNode is overridden in both subclasses, so this code
>  isn't ever executed. And, it doesn't need to be.

Done.

>  > -  // FIXME: No strict ordering.
>  > -  return false;
>  > +  if (left->NodeQueueId && right->NodeQueueId)
>  > +    return (left->NodeQueueId < right->NodeQueueId);
>  > +  return left->NodeNum < right->NodeNum;
>
>  In addition to Evan's comment, please change the NodeQueueId
>  comparison to use > instead of <. This is somewhat arbitrary, but in
>  some ad-hoc testing it seemed to give better results. In particular,
>  there are three regression tests which fail without this change due
>  to inferior scheduling.

Done.

>  Dan
>
>
>  On Apr 21, 2008, at 11:12 AM, Evan Cheng wrote:
>
> >
> > +  if (left->NodeQueueId && right->NodeQueueId)
> > +    return (left->NodeQueueId < right->NodeQueueId);
> > +  return left->NodeNum < right->NodeNum;
> >
> > Why would NodeQueueId ever be zero? Nodes that are entered into the
> > queue must have this field set, no?

Fixed. Done.

Evan wrote:
> Please do more testing on this patch. I am seeing failures:

> /Users/echeng/LLVM/llvm/Release/bin/llc  -f Output/burg.llvm.bc -o
> Output/burg.llc.s
> Assertion failed: (RemovedNum == 1 && "Not in queue!"), function
> remove, file ScheduleDAGRRList.cpp, line 1321.

> /Users/echeng/LLVM/llvm/Release/bin/llc  -f Output/sqlite3.llvm.bc -o
> Output/sqlite3.llc.s
> Assertion failed: (RemovedNum == 1 && "Not in queue!"), function
> remove, file ScheduleDAGRRList.cpp, line 1321.

Fixed. This was a bug related to the CapturePred function. It was
updating the state of the SUnit before removing it. As a result, the
comparison operators were working incorrectly and this SUnit could not
be removed properly.

With this patch, everything compiles without any problems on my machine.

The new patch is attached.

-Roman

> >
> > On Apr 18, 2008, at 4:31 AM, Roman Levenstein wrote:
> >
> >
> > > Hi Evan,
> > >
> > > 2008/4/2, Evan Cheng <evan.cheng at apple.com>:
> > >
> > > >
> > > > On Apr 2, 2008, at 1:57 AM, Roman Levenstein wrote:
> > > >
> > > >
> > > > > 2008/4/2, Evan Cheng <evan.cheng at apple.com>:
> > > > >
> > > > > >
> > > > > > On Apr 2, 2008, at 12:55 AM, Roman Levenstein wrote:
> > > > > >
> > > > > >
> > > > > > > Hi Evan,
> > > > > > >
> > > > > > > 2008/4/1, Evan Cheng <evan.cheng at apple.com>:
> > > > > > >
> > > > > > > > Please hold off checking it in for a bit. llvm tot is having
> some
> > > > > > > > problems and I'd like to get to the bottom of it first.
> > > > > > > >
> > > > > > >
> > > > > > > OK.
> > > > > > >
> > > > > > >
> > > > > > > > Also, the tie breaker is less than ideal. I think we need a
> tie-
> > > > > > > > breaker that is "the SUnit that's added to the queue is
> > > > > > > > preferred".
> > > > > > > > That means it prefers nodes which are closer to the end of
> block.
> > > > > > > > What
> > > > > > > > do you think?
> > > > > > > >
> > > > > > >
> > > > > > > Do you actually mean "the SUnit that's added to the queue LAST
> (or
> > > > > > > FIRST) is preferred"? I'll think about it.
> > > > > > >
> > > > > >
> > > > > >
> > > > > > Yep "first".  Basically, if all else being equal, let the node
> > > > > > that's
> > > > > > ready first be scheduled first. We can add a order id to SUnit
> > > > > > which
> > > > > > gets set when it's pushed into the ready queue. What do you think?
> > > > > >
> > > > >
> > > > > Makes sense. The queue should have a global "current id" counter.
> > > > > Its
> > > > > current value is assigned to each node being inserted into the ready
> > > > > queue and then incremented. When the node is removed from the queue
> > > > > for any reason, its queue order id is reset. It should be rather
> > > > > easy
> > > > > to implement.
> > > > >
> > > >
> > >
> > >
> > > >
> > > > > BTW, do you really want this queue order id in the SUnit or in a
> > > > > separate array indexed by SUnit unique ids?
> > > > >
> > > >
> > > >
> > > > It should be in SUnit since the sort functions don't have access to
> > > > ScheduleDAG members.
> > > >
> > >
> > > Please find and review the attached patch implementing:
> > > - a proper tie-breaker as discussed above.
> > > - and unmodified part for replacing the slow std::priority_queue by
> > > std::set, as it I already did before.
> > >
> > > What do you think?
> > >
> > > -Roman
> > > <ScheduleDAGRRList.patch>
> > >
> >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ScheduleDAGRRList.patch1
Type: application/octet-stream
Size: 6445 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080424/4adb19a1/attachment.obj>