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

[Tanya Lattner]

On Mar 19, 2008, at 3:44 AM, Roman Levenstein wrote:

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

Yes, it can take a long time to run. Make sure you are doing a  
release build and try just  a subset such as MultiSource/Benchmarks.  
Thats a good sanity check and will help speed up the process of  
applying your patch.

Thanks for all  your hard work!

-Tanya




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