[PATCH] [Polly][Refactor][NFC] Allow to nicely bail from ScopInfo.

[Tobias Grosser]

On 01.11.2014 02:06, Johannes Doerfert wrote:
> On 11/01, Tobias Grosser wrote:
>> On 01.11.2014 00:32, Johannes Doerfert wrote:
>>> Hi grosser, sebpop, dpeixott, simbuerg, zinob,
>>>
>>>    Each SCoP statement has a flag to indicate if the construction was
>>>    successful and if one statement couldn't be constructed the SCoP
>>>    is invalidated and removed.
>>
>> Hi Johannes,
>>
>> I very much like your approach of submitting individual self contained
>> patches. In this case, it seems the feature is fully unused.
> Part of it is unused, that is true.
>
>> I have a couple
>> of comments on bailing out in the ScopStmt, but would prefer to discuss this
>> when we have an actual use case where this could
>> be needed. Would it be OK to revisit this change if/when we have a case
>> where we might want to bail out?

> I'd appreciate it if you could at least explain what the problem is
> instead of only stating one.

There is little to say to this without knowing the use case. When 
reviewing such a patch, three questions pop up:

1) Is bailing out the only option?

2) Could we bail out earlier?

4) Is this the best way to bail out?

It is difficult to answer 1) and 2) without an actual use case. 
Regarding 2) we could possibly exchange some arguments about style, but 
nothing useful would come out here.

 > I don't want to use my spare time to write
> patches that are not committed in the end, therefore I will just give you
> one use case right now:

Thanks for the use case.

>
>    In the future we will not rely on ScalarEvlolution to compute the loop
>    bounds but do it ourselves using the affine conditions in the SCoP.

This would be great, yes.

>    However, as we can then allow more regions to be SCoPs (the ones for
>    which ScalarEvlolution cannot figure out a trip count) we run into
>    the problem that the loop trip count might be infinite (endless loop).

I understand. You mentioned this possible problem in our discussion in 
San Diego.

>    For those the scheduler will complain and there is no real benefit in
>    optimizing them anyway. Thus, we have to bail __after__ we created the
>    statement domains using the affine conditions in a SCoP.

This is indeed an interesting problem. pet (our polyhedral extractor) 
extracts happily infinite loops and the isl AST generator happily 
generates them. So I am a little surprised that the scheduler will crash 
on them.

On the other side, despites the scheduler crash I can also see that 
infinite loops may not be very interesting to optimize. So bailing makes 
sense in some way.

Going back to the beginning, I think it would make sense to include this 
patch with your actual use case, to avoid adding (temporarily) dead code 
to LLVM and to directly show in which way this code is used.

Cheers,
Tobias