Re: [PATCH] Restore previous change for gimple_phi_iterator

[Tobias Grosser <tobias at grosser dot es>]

On 07/02/2015 08:34 PM, Sebastian Pop wrote:
> On Thu, Jul 2, 2015 at 1:17 PM, Tobias Grosser <tobias@grosser.es> wrote:
> > On 07/02/2015 06:52 PM, Aditya Kumar wrote:
> > > gcc/ChangeLog:
> > >
> > > 2015-07-02  Aditya Kumar  <aditya.k7@samsung.com>
> > >              Sebastian Pop  <s.pop@samsung.com>
> > >
> > >           * graphite-sese-to-poly.c (rewrite_cross_bb_scalar_deps):
> > >          Point iterator to use_stmt.
> >
> > Hi Aditya,
> >
> > this patch does not explain what was wrong and why this change is correct.
> > Could you possibly add such an explanation.
>
> One of the code refactorings introducing phi node iterators modified
> the semantics of this code as described below ...
>
> > Best,
> > Tobias
> >
> >
> > > Bug introduced by patch:
> > > git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@217787
>
> ... here.
> If you git log grep for this commit, you would see that this patch
> reverts this "typo" introduced in a very large patch.

Sure. The corresponding change was:

-       gimple_stmt_iterator psi = gsi_for_stmt (use_stmt);
+       gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt));

Now this commit is not a pure revert. Instead of falling back to 
gsi_for_stmt, we now use gsi_for_phi(phi) and also somehow modify the 
condition above. I assume this is still correct, but as I am a little 
out of graphite, it would really help to explain (in two sentences in 
the commit message) why the previous change was wrong and how the 
behavior is now different. Something like:

"After this patch we start to iterate at the very first phi node,
whereas before this applied we skipped the PHI nodes and started 
iterating at the first actual instruciton."

Thanks,
Tobias