[llvm-commits] Speeding up instruction selection

[Roman Levenstein]

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.

 OK. Will do.


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

 This is a very good point. I was not quite sure about the sign also ;-)
 But does your comment mean that this change fixes some failures (e.g.
 burg) that were reported by Evan?
 Does it pass all the tests wih this change now? Or do I need yet to
 investigate the problems that Evan reported?

 -Roman



 >  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?
 > >
 > > Evan
 > >
 > > 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>
 > > >
 > >
 > >
 > > _______________________________________________
 > > llvm-commits mailing list
 > > llvm-commits at cs.uiuc.edu
 > > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
 > >
 >
 >