[polly] r264118 - [ScopInfo] Fix domains after loops.

[Michael Kruse via llvm-commits]

2016-04-04 11:55 GMT+02:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
>> >For the cases that trigger undefined behaviour (A [+B1]), we might
>> >want to add count2>=1 to the assumption context.  Not sure why this
>> >isn't done yet.
>>
>> Right. This could be added. I suggest to keep this in mind and to look again
>> into this after having resolved the actual bug we see.
> I don't see the need for such an assumption, the behaviour is [as you
> said] undefined. We actually had those assumptions initially and they
> caused a lot of unnecessary runtime checks. That was the reason we
> started to look for SCEVAddRec with nsw flags [see above].

In loop-succ-cond.ll, Polly doesn't know it's undefined. It's not the
assumption context.

Don't we have a separate context that we know that is true due to
otherwise undefined behaviour, but don't add runtime checks for it?


>> >In all cases, I think we should add domain lower and upper bound only
>> >together and we shouldn't allow ISL to add new contraints when leaving
>> >the loop. It might find other contraints in  [p] -> { [i0] : i0 > 0
>> >and <contraints found in the loop> }.
>>
>> I think it might be good to do what you did in your patch, but before moving
>> ahead I would like to understand if this is a correctness issue
>> or just constraints that we can make simpler.
> There are different things here.
>   1) Adding lower and upper bounds only together is not as simple as
>      there is no single "lower" and "upper" bound but only pieces which
>      cannot be looked up somewhere.

I don't understand this argument. This is what
Scop::addLoopBoundsToHeaderDomain() does.


>   2) I agree that we might not want to keep constraints from inside a
>      loop but I am not even sure about it. Since I don't think we have
>      a correctness issue either way I would like to go forward only with
>      a motivating example that profits from this.

It is a correctness issue in the LNT Adobe-C++ loop_unroll test case.


>> Your example above suggests that you have a correctness issue in mind. As
>> said above, do we already have (or can we draft) such a test case?
>>
>> > D18450 cannot help when the loop
>> >has multiple exits.
>>
>> Agreed. And for whatever reason it also did not fix the bug we see on LNT.
> That statement is not helping. AFAIK, there is little evidence that the
> additional constraints cause the lnt failure.

Huh?


> From what I understand,
> the additional constrains prevent us from statically dropping the SCoP,
> thus it mainly will hide bugs in all parts after the ScopInfo.

The SCoP _must_ be dropped because of an unconditional "error"
subregion (it calls printf). Not dropping it would either make us
transform a scop with a function call or miscompile because the error
subregion computes an escaping value.


Michael