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

[Michael Kruse via llvm-commits]

2015-11-09 4:29 GMT+01:00 Johannes Doerfert <doerfert at cs.uni-saarland.de>:
>> >   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).

Where do I get it?

>  1) Activate polly and unprofitable by default in your source. I have a
>     separate commit to do that.

OK, I was using -opt-args=-polly-detect-unproditable, it it looks like
it is ignored.

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

I was creating and committing those before DeLICM on Tobias' request.


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

To be concrete here, if we are reverting r250625, you can start
committing new features, but I will have to rebase r250625 onto that
until finally we have DeLICM working.

Reordering code generation is a larger change and from previous such
cases we had, Tobias prefers to have them fixed even before structural
changes. From what I know the only thing broken at the moment is
DominatorTree preservation. It needs to be fixed anyway, why not now?

Michael