On Mar 4, 2008, at 10:37 AM, Roman Levenstein wrote:
>>
>> I thought those were approved? We understanding std::set is probably
>> the best choice of ADT for now. Thanks!
>
> OK. I'll commit them tomorrow.
Great! Note that I did have one comment on the ScheduleDAGList.cpp
patch.
>
> But are you sure about the second one for the ScheduleDAGRRList?
> I mean this: http://lists.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20080303/059078.html
I just took a look at this one. Here are my comments.
> + // This may break the ordering in the priority queue.
> PredSU->CycleBound = std::max(PredSU->CycleBound,
> I->Dep->Cycle + PredSU->Latency);
This seems dangerous. After this is done, updateNode is called,
which attempts to remove the node from the set, but if the node has
changed
in a way that's significant to the comparison function, that remove
may not
work properly. It looks like the only way to be safe is to remove the
node
from the queue before making any modifications.
> + for (unsigned i = 0, e = Nodes.size(); i != e; ++i) {
> + Queue.insert(Nodes[i]);
> + }
This could be simplified with
Queue.insert(Nodes.begin(), Nodes.end());
> + SUnit *V = *Queue.rbegin();
> + Queue.erase(--Queue.end(),Queue.end());
This could be simplified with
Queue.erase(prior(Queue.end());
or even
iterator i = prior(Queue.rbegin());
SUnit *V = *i;
Queue.erase(i);
`prior' is defined in llvm/ADT/STLExtras.h.
> - unsigned LBonus = (SumOfUnscheduledPredsOfSuccs(left) == 1) ?
2 : 0;
> - unsigned RBonus = (SumOfUnscheduledPredsOfSuccs(right) == 1) ?
2 : 0;
> + unsigned LBonus = (SumOfUnscheduledPredsOfSuccs(left,1) == 1) ?
2 : 0;
> + unsigned RBonus = (SumOfUnscheduledPredsOfSuccs(right,1) == 1) ?
2 : 0;
It's not clear to me what this change is doing. Well, the original
code isn't
quite clear to me either :-}. Is this a heuristic change?
Dan