[llvm-commits] Speeding up instruction selection

[Dan Gohman]

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.

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

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