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

Mon Apr 4 07:09:26 PDT 2016

On 04/04, Michael Kruse wrote:
> 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.
Polly knows it is undefined because we saw an SCEVAddRec of that loop
that did have the nsw flag. Check the SCEVAffinator::hasNSWAddRecForLoop
function and its call side in ScopInfo.cpp.

> 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?
We don't have such a context, mainly because we "do not need it" but also
because "it would always be incomplete". I do not want to say we could
not use that for user feedback but it is not something I would always
compute just for the sake of it. Nevertheless, I am strongly supporting
Polly to exploit undefined behaviour.

> >> >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.
Close but it doesn't provide a "upper bound value" and it uses the
propagated domain to come up with some "upper bound function" in the
first place. This function is actually close to the
propagateDomainConstrains function but it also deals with the fact that
there are different loop iterations involved. As this is a complicated
topic we might want to start a new thread for this where you can explain
the problem of the current approach [only if it can be isolated from
this thread].

> >   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.
So you are positive that the domain is __incorrect__ for this test case
and that it is the root cause of the failing test? If so I will take a
closer look into that test [though it seems we pass all lnt test since
today]. 

By the way, what were the symptoms of that failure? Compile or execution
time failure? Violated assertion or just crash? There is a known bug in
the RTCs [PR26683] that is currently my goto candidate if we crash at
execution time but should not. However, we also have/had a couple of
other bugs that are open [PR27013, PR27195] or have been fixed recently
[r265286, r265280, r265261, r265132]. Maybe one of them was the culprit.

> >> 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?
Correct me if I am wrong but I understood the chaing of events like like this:
  1) We had LNT failures in some build.
  2) Your [FIX] Domain ... patch resulted in a "different" domain that
     caused us to dismiss SCoPs statically that would/should have a
     "false" RTC at runtime anyway but for which we could not show that
     with the "former" domain.
  3) The LNT tests were now passing.

If this listing of events is "accurate" I am not convinced that the problem was
actually the domain. Instead I think it might have been in the code generation
that was triggered before but not after your patch.  While code generation
should not change the semantics as the RTCs should be false anyway, it is
possible that this happend. A bug in the RTCs or any other code generation
part could cause problems even if the domains are fine and it would be hidden
if we can now statically drop the SCoPs.

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

We never have to "drop" a SCoP statically, that is just an optimization we
perform. What we have to do is to make sure the "Polly optimized version" is
only executed if all assumptions were true at runtime. If we can statically
show that this will never be the case there is no need in generating the "Polly
optimized version", but it should never be a problem to do so as the RTCs are
supposed to guard the execution. Do not get me wrong, we have bugs in the RTCs
and the code generation all the time, thus this is more a theoretical argument
but it is one nevertheless.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: Digital signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160404/dde7fcac/attachment.sig>