[llvm-commits] Patch for review: Speeding up ScheduleDAG computations

[Evan Cheng]

Some comments.

+
+  if (!IsRemoved)
+    // fix the ordering here
+    AvailableQueue->updateNode(PredSU);

Please start comment with a capital letter. :-)

-    virtual void updateNode(const SUnit *SU) {}
+    virtual void updateNode(const SUnit *SU) {
+      remove((SUnit *)SU);
+      push((SUnit *)SU);
+    }

Rather than casting away the constness. It's probably ok to change the  
prototype instead.

-      Queue.push(U);
+	Queue.insert(U);  <=== tab here
      }
      void push_all(const std::vector<SUnit *> &Nodes) {
-      for (unsigned i = 0, e = Nodes.size(); i != e; ++i)
-        Queue.push(Nodes[i]);
+      for (unsigned i = 0, e = Nodes.size(); i != e; ++i) {
+	Queue.insert(Nodes[i]);  <=== tab here
+      }

No tabs please.

-  // FIXME: No strict ordering.
-  return false;
+  return (left->NodeNum < right->NodeNum);

I think this is ok for now. But perhaps we can add a field to SUnit to  
track the relative temporal order of when nodes are inserted in the  
queue. This should then be changed to prefer nodes that were entered  
earlier rather than NodeNum which can be arbitrary. What do you think?  
This can be a later patch.

Thanks,

Evan


On Mar 3, 2008, at 9:18 AM, Roman Levenstein wrote:

> Hi Dave,
>
> 2008/3/3, David Greene <dag at cray.com>:
>> On Friday 29 February 2008 17:49, Evan Cheng wrote:
>>> Let's try to solve one problem at a time.
>>>
>>> 1. Roman's depth / height patch calculation is obviously goodness.
>>> Please commit that first.
>>> 2. It's probably safe to eliminate the "special tie-breaker" from  
>>> top
>>> down sorting function.
>>> 3. We will need to fix up the sorting functions so strict ordering  
>>> is
>>> preserved. We need to do so without breaking any of the existing  
>>> test
>>> case. "return left->NodeNum < right->NodeNum;" doesn't. :-( What  
>>> does
>>> "return false" mean? Is it favoring nodes that are inserted earlier?
>>> 4. It might be wise to add some debugging code so we can tell where
>>> things start diverge.
>>> 5. It would be nice to be able to rebalance the heap. Roman and  
>>> David
>>> can fight it out when we get to that point. :-)
>>
>>
>> Sounds good.  I don't know what "fight it out" means, however.  :)
> Me either ;)
>
>> It's more than "nice" to rebalance the heap.  It's necessary if we
>> want to avoid traps when _GLIBCXX_DEBUG is enabled.  And we
>> want to do that because it finds lots of bugs.
> Totally agree.
>
> Here is a proposed patch for ScheduleDAGRRList.cpp. It contains the
> following changes:
> 1) Uses std::set instead of the priority queue. This makes removal of
> nodes very fast and removes a bottleneck
> 2) sorting functions use now strict ordering
> 3) According to your proposal, it takes node priority updates into
> account and updates the priority queue by removing and re-inserting
> the updated element.
> 4) SumOfUnscheduledPredsOfSuccs is slightly changed to avoid useless
> computations. This saves a LOT OF time on big basic blocks.
>
> Please review and test, if possible. I have problems with running the
> llvm-test test-suite. When I to run it (and keep in mind, I never did
> it before), I always get the following error messages on my X86/Ubuntu
> system:
> make[4]: *** No rule to make target
> `Output/sse.expandfft.linked.rbc', needed by
> `Output/sse.expandfft.linked.bc'.  Stop.
>
>
> -Roman
> < 
> ScheduleDAGRRList 
> .patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits