[llvm-commits] Fwd: Patch for review: Speeding up ScheduleDAG computations
Dan Gohman
gohman at apple.com
Tue Mar 4 14:39:55 PST 2008
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
More information about the llvm-commits
mailing list