[llvm-commits] Fwd: Speeding up instruction selection

[Roman Levenstein]

Oops. You mean a totally different patch, that happen to use std::set
as well ;-)
May be we should start using different subjects for different
"speeding up" patches?
This would make understanding of the conversation and following this
thread a bit easier ;-)

-Roman

2008/4/23 Roman Levenstein <romix.llvm at googlemail.com>:
> OK. I'll have a look at it.
>
>  Looking at the error message,  it seems to me that it happens with the
>  USE_QUEUE version of the patch.
>  What about the NO_QUEUE version? Does it also fail?
>
>  Unfortunately, I cannot check it at the moment, because I don't have
>  access to my development machine, but will do tomorrow.
>
>  -Roman
>
>  2008/4/23 Evan Cheng <evan.cheng at apple.com>:
>
>
> > Hi Roman,
>  >
>  >  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.
>  >
>  >  Thanks,
>  >
>  >  Evan
>  >
>  >
>  >
>  >  On Apr 21, 2008, at 12:44 PM, Evan Cheng wrote:
>  >
>  >  >
>  >  > On Apr 21, 2008, at 11:54 AM, Roman Levenstein wrote:
>  >  >
>  >  >> Hi Evan,
>  >  >>
>  >  >> Good point. This code is here due to the historical reasons ;-) I
>  >  >> introduced it when I was experimenting with different alternatives
>  >  >> and
>  >  >> then forgot to remove it... The commit version will not contain it.
>  >  >>
>  >  >> Any further comments on this patch? Can I commit it or is it too
>  >  >> early?
>  >  >
>  >  > Please hold off committing for a few days until I have a chance to
>  >  > evaluate the performance impact. Thanks.
>  >  >
>  >  > Evan
>  >  >
>  >  >>
>  >  >>
>  >  >> - Roman
>  >  >>
>  >  >> 2008/4/21, Evan Cheng <evan.cheng at apple.com>:
>  >  >>
>  >  >>> +  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
>  >  >
>  >  > _______________________________________________
>  >  > llvm-commits mailing list
>  >  > llvm-commits at cs.uiuc.edu
>  >  > http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>  >
>  >  _______________________________________________
>  >  llvm-commits mailing list
>  >  llvm-commits at cs.uiuc.edu
>  >  http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>  >
>