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

[Roman Levenstein]

Hi,

2008/3/18, Dan Gohman <gohman at apple.com>:
> +  /// Functions for preserving the topological ordering
>  +  /// even when new edges are inserted online.
>  +  /// This allows for very fast isReachable implementation.
>  +  /// The idea of the algorithm is taken from
>  +  /// "Online algorithms for managing the topological order of
>
> +  ///  a directed acyclic graph" by David J.Pearce and Paul H.J. Kelly
>
>
> In your email you mentioned that the algorithm used is the MNR
>  algorithm. Can you mention that in this comment? Also, the
>  paper cites a different paper as the original source for the
>  MNR algorithm. Can you clarify that, possibly mentioning both
>  papers here?

Done.

>  I don't see any cosmetic issues, beyond what Tanya brought up.
>  With those addressed, I think you can go ahead and check this
>  in.

> A couple comments on this pach.

>- Have you tested this on llvm-test (test-suite in svn)? Any
> significant regressions?

llvm/test passes without any problems
Trying to run the llvm-test test-suite. But it is huge and takes hours to run.

> - 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.

Done.

> - Comments should have proper punctuation. :)

Done. Hopefully :-)

> - 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?

Done. There is a long high-level comment in the sources now.
Hopefully, it gives the idea.

> - Please be consistent with how you name your functions. Some are
> starting with lower case and some with upper case.

Done.

>- 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.

Done. The reason for my naming was to use exactly the same names as in
the paper to make it easier to follow the implementation.

> I haven't studied the algorithm in detail, but the previous
>  code clearly leaves room for improvement, and it sounds like
>  you've done reasonable testing. We'll continue to review the
>  code after it's checked in.

>  BTW, I still haven't been able to reduce the problem in 176.gcc
>  with your queue ordering patch. Is this something you are
>  able to look into? We'd like to understand what's going on
>  there before the patch is comitted. I'm continuing to try to
>  narrow it down, but I wasn't able to get bugpoint to help, so
>  it might take me a while.

Hmm. I don't have access to SPEC testcases. Where and how can I obtain them?

>  Also, what's the status of your isel patch? Last I saw there
>  were some changes discussed on the list. I'm just checking to
>  make sure you're not waiting for something.

I guess you mean that SDNode->Uses related patch? I incorporated
cosmetic changes suggested by Evan and will send them in a separate
email. But I'm still not sure if it doesn't break anything. And I
really have troubles running a complete llvm-test suit because it
takes ages to complete on my machine. I'd really appreciate if someone
with a faster machine could run llvm-test through it.

For the time-being, I attach the updated patch.

-Roman
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ScheduleDAGRRList.isReachable.MNR.patch
Type: text/x-patch
Size: 20020 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20080319/6861144d/attachment.bin>