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

[Dan Gohman]

Hi Roman,

I've tested this patch, and it passes. Please commit this.

Thanks!

Dan

On Mar 19, 2008, at 3:44 AM, Roman Levenstein wrote:
> 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
> < 
> ScheduleDAGRRList 
> .isReachable.MNR.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits