[llvm-commits] Patch for review: Speeding-up isReachable computation, fix for Bugzilla #1835

[Tanya Lattner]

A couple comments on this pach.

- Have you tested this on llvm-test (test-suite in svn)? Any  
significant regressions?
- Please use doxygen style comments for each individual function. So  
for example: InitDAGTopologicalSorting, SFS, Shift, Allocate, etc  
should all have their own comment describing what they do.
- Comments should have proper punctuation. :)
- I don't want to have to go read the paper to understand what is  
going on. Can you add a high level comment explaining the algorithm  
and the steps involved?
- Please be consistent with how you name your functions. Some are  
starting with lower case and some with upper case.
- Feel free to spell out the name for std::vector<int> i2n and n2i.  
Call them IndextoNode or something. Its easier to read in my opinion.
- In isReachable(), I'd prefer ub and lb to be spelled out. Makes  
code reading easier. You can also just do int ulb = .. to be  
consistent with llvm style instead of declaring at the top.

Thanks!
-Tanya


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

> Hi,
>
> I've implemented some improvements related to the computation of  
> isReachable.
> As reported by me in Bugzilla #1835
> (http://llvm.org/bugs/show_bug.cgi?id=1835), this function may consume
> 99% of compilation time on very big scheduler DAGs.
>
> The reason for this bad performance is that current algorithm is very
> naive and scans all nodes of the DAG in the worst case.
>
> I implemented a better algorithm based on the paper "Online algorithms
> for maintaining the topological order of a directed acyclic graph" by
> David J.Pearce and Paul H.J.Kelly. This is the MNR algorithm. It has a
> linear time worst-case and performs much better in most situations.
>
> The paper can be found here:
> http://fano.ics.uci.edu/cites/Document/Online-algorithms-for- 
> maintaining-the-topological-order-of-a-directed-acyclic-graph.html
>
> I tested the new algorithm comparing it with the old one side-by-side
> and they seem to produce the same results.
>
> Tests on very big  input files with tens of thousands of instructions
> in a BB, e.g. big4.bc use-case from Duraid's testsuit, indicate huge
> speed-ups (up to 10x compilation time improvement) compared to the
> current version.
>
> The performance of BURR scheduler seems to be now similar to TDRR
> scheduler even on the corner-cases mentioned in the Bugzilla. So, I'd
> consider it to be a proper fix.
>
> No new regressions are introduced into the llvm/test.
>
> Please review, test and tell if it is OK for submission.
>
> -  
> Roman<ScheduleDAGRRList.isReachable.patch>____________________________ 
> ___________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits