[llvm-commits] Fwd: Speeding up instruction selection

[Roman Levenstein]

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
>