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

Sun Nov 8 20:40:33 PST 2015

On 11/09, Michael Kruse wrote:
> 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?
He send it to one of the mailing lists I think. I don't have it because
I couldn't find though...

> >  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.
I know. I told that Tobias in SJ already because he reviewed those commits.
We agreed on not continuing this very early precommiting from now on
further.

> >> >   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.
That is correct. And I would even have to rebase my new features to the
version without r250625 but at least we would not be blocked anymore. If
we would not have put the profitable option in the original buildbot we
would have detected all these errors earlier and, as the other LLVM
folks, we should revert broken commits if they are not fixed right away.
A broken build should never be a normal state.

> 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?
Don't get me wrong, I want a clean built too, but I want it now and not
at some point soon. To this end I suggested reverting that commit now
and restructuring everything afterwards. My problem with your argument
is that __I don't know what is broken at the moment__. I fixed 2 runtime
bugs today as the compile time bugs all kinda look the same. That is
also the reason I attached 4 test cases to one bug even though it turns
out (by your analysis) they have different causes.
-------------- 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/92cc884b/attachment.sig>