[polly] scop detection: detect regions bottom up

[Tobias Grosser]

On 06/12/2013 10:27 PM, Star Tan wrote:
> Hi Sebastian,
>
>
> Your work is cool. I am also working on reducing Polly compile-time for my GSOC 2013 project (https://gist.github.com/tanstar/5508153)
> I think we can work together by exchanging our ideas ~

Yes, it is great that we got some traction in this direction and that we 
can work on this collaboratively.

> At 2013-06-13 05:40:31,"Sebastian Pop" <spop at codeaurora.org> wrote:
>
>> Tobias Grosser wrote:
>>> Also, in terms of speeding up scop detection. My guess is that the
>>> largest speedup would come from detecting scops bottom up, starting
>>> from small regions to bigger regions. We could then skip analyzing
>>> regions as soon as we found a construct in a smaller region that
>>> would
>>> also invalidate all regions containing the smaller region.
>>
>> Here is a patch that makes scop detection to work bottom up.  On my huge C++
>> benchmark, the attached patch gets the overall sequential time spent in Polly
>>from 2151 seconds to 1573 seconds.
> Cool, the compile time is reduced by about 1/4. Could you tell me which benchmark do you use? Was it in LLVM test-suite?
> I have set up a LNT-based Polly performance tester using LLVM test-suite. Maybe I can help to evaluate your patch on these benchmarks tonight :)

Yes, please. It would be very valuable to get performance numbers on the 
open source test-suite.

Just to see if this is beneficial in all cases or if there are cases 
where going the other direction is actually better.

>> Also note that there still are redundant computations in the case of nested
>> valid regions: we would start calling isValidRegion on the innermost region
>> containing a loop, then supposing that the parent region is composed of only
>> valid regions, we would call again isValidRegion and that would walk again over
>> all the subregions, validating the CFG, loops, and every instruction again.
>>
>> With the attached patch there are 4 fails:
>>
>> Failing Tests (4):
>>     Polly :: polybench/linear-algebra/solvers/lu/lu_with_param.ll
>>     Polly :: polybench/linear-algebra/solvers/lu/lu_without_param.ll
>>     Polly :: polybench/linear-algebra/solvers/ludcmp/ludcmp_without_param.ll
>>     Polly :: polybench/stencils/jacobi-2d-imper/jacobi-2d-imper_with_param.ll
>>
>> These fails are due to the fact that we do not detect a maximal region anymore:
>> first, scop detection splits region exit blocks when the exit block has multiple
>> exit edges, and then these new basic blocks that we inserted do not belong to
>> any region, and so we end up not discovering a maximal region anymore.
>>
>> I think we can add the newly created blocks to the region info.
>>
>> Another solution, and probably more difficult, is to avoid invalidating the
>> region info by trying to not split the blocks ending a region.  I'm not sure
>> about the impact of such a change on the codegen region versioning.
> Can I do anything to help you? Maybe I can help to fix this problem by avoiding invalidating the region info.

I let Sebastian answer this, but here some advices from me:

1) Feel free to review patches

Reviewing patches is very important and I encourage you to review the 
patches Sebastian posts. Even if you are not an expert in every detail, 
just review the parts of the patch that you understand and ask for parts 
that are not clear. This either shows weaknesses in the patch or it will 
help you to get more into Polly.

2) Test coverage

For some patches you may ask yourself if they actually work as intended 
on a certain corner case. In case you find a case you have been unsure 
about, please just write a test case and propose it for inclusion. Such 
tests are very valuable when looking at the code again in the future. 
Also, when reviewing patches, one of the important points is to mentally 
go through the patch and ask yourself if you can write a test case that 
would break the patch. If we can not find one, the patch is
probably ready to go in.

Cheers,
Tobi