[llvm-commits] Speeding up instruction selection

Dan Gohman gohman at apple.com
Mon Apr 21 08:47:05 PDT 2008


On Fri, April 18, 2008 11:04 pm, Roman Levenstein wrote:
> Hi Dan, Hi Evan,
>
> 2008/4/19, Dan Gohman <gohman at apple.com>:
>>
>>  On Apr 16, 2008, at 9:19 AM, Roman Levenstein wrote:
>>  >
>>
>> > BTW, Dan, have you had any time to look at the patch introducing
>>  > queues instead of heaps in the target specific instruction selectors?
>>
>>
>> Is this the ScheduleDAGRRList patch that changes std::priority_queue
>>  to std::set? I know you checked in part of those changes already;
>>  could you send me an updated patch with just the outstanding changes?
>
> No. I don't mean the ScheduleDAGRRList.pacth. For this one, I proposed
> the remaining patch just with the outstanding patches yesterday (see
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080414/061236.html).
> And it is also about using the std::set :-)

Ok. Thanks for the reminder.

>
> The patch for instruction selectors was proposed by me here:
> http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080303/059215.html
> It is related to the Select() and SelectRoot implementations inside
> the target-specific code selectors generated by tablegen.
>
> Regarding my questions about std::set vs std;;multiset in the mentioned
> patch:
> Having looked into it again, I have the impression that std::set could
> be used and there is no need for std::multiset. Basically, the this
> code:
>   void AddToISelQueue(SDOperand N) DISABLE_INLINE {
>    int Id = N.Val->getNodeId();
>    if (Id != -1 && !isQueued(Id)) {
>    ...
> }
> ensures that only one node with a given NodeId can be inserted into
> the ISelQueue.
>
> One possible further improvement, I'm thinking about is the following:
> Why do we use a sorted data structure (e.g. sorted vector, set) for
> the ISelQueue at all?
>
> 1) If we look into the SelectRoot logic, it basically takes the node
> with the SMALLEST NodeId from the pending selection queue.
> 2) More over, it seems to me (please, confirm if it is true!) that the
> next node taken for selection in the loop inside the SelectRoot, at
> any point in time, has a NodeId that is GREATER than the NodeId of the
> last selected node.
> 3) Even if the selection of the node puts new nodes into the selection
> queue, they ALWAYS have NodeIds GREATER than that of this node.
> Therefore the property (2) is preserved.
>
> I checked (2) and (3) by adding some debug output to the SelectRoot.
> Only one test of all llvm/test test-cases does not fulfill this
> assumptions and even there it is not quite obvious if it is a
> contradiction to (2) and (3) or a bug.
>
> So, assuming that properties (2) and (3) do hold, the following can be
> probably done:
> - We iterate over the NodeId numbers from 0 till the DAGSize.
> - We check if a Node with a given NodeId (by looking into the
> TopOrder???), if isQueued(NodeId) is true, i.e. this is in the queue
> - We then check if isSelected(NodeId) is true or false and call
> Select() for this node, if it is not selected yet.
> - If a new node needs to be inserted into the queue, only
> setQueued(NodeId) is called
>
> With these changes, no online sorting would be required at all, which
> is a great improvement over the current approach.
>
> What do you think about it?  Is it a correct analysis?

It sounds plausible :-).

I think you have to watch out for the case where nodes get created
during the isel process. I'm aware of one place that does this, in
X86ISelDAGToDAG.cpp around line 870. It appears to set the NodeId
values for the new nodes, but it doesn't currently update TopOrder
or call ReplaceAllUses; perhaps that could be fixed.

Offhand, I don't know if there are any other places. I also don't
know if the current code was written with grander plans in mind
that would require a priority queue instead of just using the
pre-computed topological ordering.

Dan





More information about the llvm-commits mailing list