[polly] r252301 - Fix reuse of non-dominating synthesized value in subregion exit

[Johannes Doerfert via llvm-commits]

On 11/09, Michael Kruse wrote:
> 2015-11-07 6:40 GMT+01:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
> >
> > This actually caused 24 more failures, see:
> >   http://lab.llvm.org:8011/builders/perf-x86_64-penryn-O3-polly-unprofitable/builds/557
> >
> > Before we were at 67, after at 91 and after r97986 we are now at 75,
> > see:
> >   http://lab.llvm.org:8011/builders/perf-x86_64-penryn-O3-polly-unprofitable/builds/569
> >
> > This brings me to several problems I would like to discuss:
> >   1) Our "fixes" are not checked to be beneficial for more then the test
> >      case that was attached to the bug report (see above).
> 
> 
> I usually test my fixes on standard lnt (without
> -polly-process-unprofitable) before committing.
I do the same thing, usually. However, as we are still debugging the
unprofitable bot we have to test it with unprofitable activated,
otherwise we will not be informed if we broke something there (which is
the only interesting information we will get from that bot). 

I found the problem described above by chance and searched for the
commit that increased the number of broken tests manually...

> >   2) The number of people looking at the lnt run and extracting tests is
> >      to little. Tobias complained in SJ about that, now it just
> >      shifted..
> 
> I may need some help on how to use bugpoint effectively. My reduced
> cases are never as small as yours.
-1) Configure polly such that it is linked in all tools.
 0) Apply Tobias bugpoint patch (I haven't but it works ok most of the
    times).
 1) Activate polly and unprofitable by default in your source. I have a
    separate commit to do that.
 2) Create a plain IR file from your input (e.g., remove the -O3 from the
    lnt run line and add -emit-llvm). Now your output file is a bitcode file
    that we will simplify.
 3) run "bugpoint -O3 <file>" and hope the result will crash with the
    same kind of error and is small enough to understand.

> >   3) We try to fix things one by one while the underlying problem is not
> >      exposed. Lately there are mainly dominance problems in non-affine
> >      regions (we see the 4th or 5th bug now). While they are all fixed
> >      by small changes to the code, it seems the changes always only
> >      fix that test case and sometimes even make things worse (see 1)). I
> >      would suggest to go back to the drawing board here and revert the
> >      change that introduced these problems (changed the scalar
> >      reload/store locations). Especially since we commited it
> >      preemptively and now might not need it anyways. Please bring
> >      forward counter arguments to this proposal.
> 
> For DeLICM by reusing array elements that are overwritten, lifetime
> analysis needs to be done. In order to not have to analyze the
> lifetime within statements, we need to ensure that scalars are only
> written/read before/after the statement itself. Hence, we will still
> need it (and at least some part of http://reviews.llvm.org/D13676).
> 
> Some of the bugs introduced by r250625 were just hidden before (like
> the broken repair of the dominance tree) and others would just come up
> again in other form which might be even harder to track because it
> would miscompile instead of us getting a compile-time assertion fail
> (r250625 was before we introduced the -polly-process-unprofitable
> buildbot). Maintaining patches off-tree is also harder as it requires
> regular rebasing.
Let's not go in the DeLICM discussion again here. If we at some point
commit something that needs this change, we can apply it, but at the
moment it just makes our lives harder. A new codegen structure might 
solve the problem I was describing here too, namely that we apply these
local patches to fix one corner case and at some point loose the
structure in our code which will cause new corner cases to appear.

> >   4) We __all__ have to get our priorities straight. If these bots are
> >      not green we cannot (or at least should not) commit anything new,
> >      though there is already new stuff pending for a while now that
> >      needs to be reviewed and decided on. The longer we wait the more
> >      work we all have...
> 
> "Just" the rebase to the (supposed to be small) bugfix patches.
> Rebasing to new features is harder. I feel this would just move the
> rebase work from one person to another.
I don't get it. What rebase work? If you talk about different features
that are ready or under construction and race with each other, that will
not go away either way. Having a green bot at least allows to commit a
new feature, though, the earlier we start commiting stuff the less
rebase will be necessary at the end.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 213 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20151109/f99a7af9/attachment.sig>