| Message ID | 1435855936-28694-1-git-send-email-hiraditya@msn.com |
|---|---|
| State | New |
| Headers | show |
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. Best, Tobias > > Bug introduced by patch: > git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@217787 > --- > gcc/graphite-sese-to-poly.c | 7 +++---- > 1 file changed, 3 insertions(+), 4 deletions(-) > > diff --git a/gcc/graphite-sese-to-poly.c b/gcc/graphite-sese-to-poly.c > index 271c499..78f10e4 100644 > --- a/gcc/graphite-sese-to-poly.c > +++ b/gcc/graphite-sese-to-poly.c > @@ -2458,11 +2458,10 @@ rewrite_cross_bb_scalar_deps (scop_p scop, gimple_stmt_iterator *gsi) > handle_scalar_deps_crossing_scop_limits (scop, def, stmt); > > FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, def) > - if (gimple_code (use_stmt) == GIMPLE_PHI > - && (res = true)) > + if (gphi *phi = dyn_cast <gphi *> (use_stmt)) > { > - gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); > - > + res = true; > + gphi_iterator psi = gsi_for_phi (phi); > if (scalar_close_phi_node_p (gsi_stmt (psi))) > rewrite_close_phi_out_of_ssa (scop, &psi); > else >
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. Thanks, Sebastian >> --- >> gcc/graphite-sese-to-poly.c | 7 +++---- >> 1 file changed, 3 insertions(+), 4 deletions(-) >> >> diff --git a/gcc/graphite-sese-to-poly.c b/gcc/graphite-sese-to-poly.c >> index 271c499..78f10e4 100644 >> --- a/gcc/graphite-sese-to-poly.c >> +++ b/gcc/graphite-sese-to-poly.c >> @@ -2458,11 +2458,10 @@ rewrite_cross_bb_scalar_deps (scop_p scop, >> gimple_stmt_iterator *gsi) >> handle_scalar_deps_crossing_scop_limits (scop, def, stmt); >> >> FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, def) >> - if (gimple_code (use_stmt) == GIMPLE_PHI >> - && (res = true)) >> + if (gphi *phi = dyn_cast <gphi *> (use_stmt)) >> { >> - gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); >> - >> + res = true; >> + gphi_iterator psi = gsi_for_phi (phi); >> if (scalar_close_phi_node_p (gsi_stmt (psi))) >> rewrite_close_phi_out_of_ssa (scop, &psi); >> else >> >
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
On Thu, Jul 2, 2015 at 7:34 PM, Sebastian Pop <sebpop@gmail.com> 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. > How about a testcase or 2 or mentioning if it is covered by existing testcases ? And Aditya you may find it instructive to read this https://gcc.gnu.org/contribute.html#patches regards Ramana > Thanks, > Sebastian > >>> --- >>> gcc/graphite-sese-to-poly.c | 7 +++---- >>> 1 file changed, 3 insertions(+), 4 deletions(-) >>> >>> diff --git a/gcc/graphite-sese-to-poly.c b/gcc/graphite-sese-to-poly.c >>> index 271c499..78f10e4 100644 >>> --- a/gcc/graphite-sese-to-poly.c >>> +++ b/gcc/graphite-sese-to-poly.c >>> @@ -2458,11 +2458,10 @@ rewrite_cross_bb_scalar_deps (scop_p scop, >>> gimple_stmt_iterator *gsi) >>> handle_scalar_deps_crossing_scop_limits (scop, def, stmt); >>> >>> FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, def) >>> - if (gimple_code (use_stmt) == GIMPLE_PHI >>> - && (res = true)) >>> + if (gphi *phi = dyn_cast <gphi *> (use_stmt)) >>> { >>> - gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); >>> - >>> + res = true; >>> + gphi_iterator psi = gsi_for_phi (phi); >>> if (scalar_close_phi_node_p (gsi_stmt (psi))) >>> rewrite_close_phi_out_of_ssa (scop, &psi); >>> else >>> >>
On Thu, Jul 2, 2015 at 1:44 PM, Tobias Grosser <tobias@grosser.es> wrote: >> 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 IMO the patch restores the semantics, so it is a revert to some syntax changes: in the past we had this: > - gimple_stmt_iterator psi = gsi_for_stmt (use_stmt); that is get me an iterator pointing on the use_stmt. After our patch we get the same semantics back (modulo some change in function names, c++-ification, etc.) gphi *phi = dyn_cast <gphi *> (use_stmt) gphi_iterator psi = gsi_for_phi (phi); that is again an iterator pointing on the use_stmt. The patch at r217787 was incorrectly initializing the iterator to point at the beginning of the phi nodes in the BB of the use_stmt: > + gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); This makes no sense, as then we would start processing a random phi node and would fail to insert an array for a virtual phi node. Sebastian > 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
On 07/02/2015 09:03 PM, Sebastian Pop wrote: > On Thu, Jul 2, 2015 at 1:44 PM, Tobias Grosser <tobias@grosser.es> wrote: >>> 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 > > IMO the patch restores the semantics, so it is a revert to some syntax changes: > in the past we had this: > >> - gimple_stmt_iterator psi = gsi_for_stmt (use_stmt); > > that is get me an iterator pointing on the use_stmt. > After our patch we get the same semantics back (modulo some change in > function names, c++-ification, etc.) > > gphi *phi = dyn_cast <gphi *> (use_stmt) > gphi_iterator psi = gsi_for_phi (phi); > > that is again an iterator pointing on the use_stmt. > > The patch at r217787 was incorrectly initializing the iterator > to point at the beginning of the phi nodes in the BB of the use_stmt: > >> + gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); > > This makes no sense, as then we would start processing a random phi node > and would fail to insert an array for a virtual phi node. Thanks. I am a little slow today. The patch looks indeed correct. Maybe you could add this explanation to the commit message and also add a test case as Ramana suggested. Tobias
On Thu, Jul 2, 2015 at 2:03 PM, Ramana Radhakrishnan <ramana.gcc@googlemail.com> wrote: > How about a testcase or 2 or mentioning if it is covered by existing > testcases ? The patch fixes a test in testsuite/gcc.dg/graphite/ when removing the use of limit_scops(). Maybe the commit message could contain the name of the test that it fixed. The patch that removes limit_scops() is in my opinion trivial, and will be submitted for review once we fixed all the errors it can cause (code gen, scop translation to polyhedral, etc.) We will also fix bootstrap with graphite enabled, and then we will fix all problems in bootstrap with limit_scops() removed. I will also add a buildbot tracking nightly bootstraps with -floop-* and -fgraphite-identity. > And Aditya you may find it instructive to read this > https://gcc.gnu.org/contribute.html#patches > Agreed. Thanks for the feedback. Sebastian > regards > Ramana > > >> Thanks, >> Sebastian >> >>>> --- >>>> gcc/graphite-sese-to-poly.c | 7 +++---- >>>> 1 file changed, 3 insertions(+), 4 deletions(-) >>>> >>>> diff --git a/gcc/graphite-sese-to-poly.c b/gcc/graphite-sese-to-poly.c >>>> index 271c499..78f10e4 100644 >>>> --- a/gcc/graphite-sese-to-poly.c >>>> +++ b/gcc/graphite-sese-to-poly.c >>>> @@ -2458,11 +2458,10 @@ rewrite_cross_bb_scalar_deps (scop_p scop, >>>> gimple_stmt_iterator *gsi) >>>> handle_scalar_deps_crossing_scop_limits (scop, def, stmt); >>>> >>>> FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, def) >>>> - if (gimple_code (use_stmt) == GIMPLE_PHI >>>> - && (res = true)) >>>> + if (gphi *phi = dyn_cast <gphi *> (use_stmt)) >>>> { >>>> - gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); >>>> - >>>> + res = true; >>>> + gphi_iterator psi = gsi_for_phi (phi); >>>> if (scalar_close_phi_node_p (gsi_stmt (psi))) >>>> rewrite_close_phi_out_of_ssa (scop, &psi); >>>> else >>>> >>>
On 07/02/2015 09:09 PM, Sebastian Pop wrote: > On Thu, Jul 2, 2015 at 2:03 PM, Ramana Radhakrishnan > <ramana.gcc@googlemail.com> wrote: >> How about a testcase or 2 or mentioning if it is covered by existing >> testcases ? > > The patch fixes a test in testsuite/gcc.dg/graphite/ when removing the > use of limit_scops(). > Maybe the commit message could contain the name of the test that it fixed. Right. I think just adding the missing information to the commit message will be enough. > The patch that removes limit_scops() is in my opinion trivial, and > will be submitted for review once we fixed all the errors it can cause > (code gen, scop translation to polyhedral, etc.) > We will also fix bootstrap with graphite enabled, and then we will fix > all problems in bootstrap with limit_scops() removed. > I will also add a buildbot tracking nightly bootstraps with -floop-* > and -fgraphite-identity. Sounds great. Nice to see more graphite activity again. Best, Tobias
diff --git a/gcc/graphite-sese-to-poly.c b/gcc/graphite-sese-to-poly.c index 271c499..78f10e4 100644 --- a/gcc/graphite-sese-to-poly.c +++ b/gcc/graphite-sese-to-poly.c @@ -2458,11 +2458,10 @@ rewrite_cross_bb_scalar_deps (scop_p scop, gimple_stmt_iterator *gsi) handle_scalar_deps_crossing_scop_limits (scop, def, stmt); FOR_EACH_IMM_USE_STMT (use_stmt, imm_iter, def) - if (gimple_code (use_stmt) == GIMPLE_PHI - && (res = true)) + if (gphi *phi = dyn_cast <gphi *> (use_stmt)) { - gphi_iterator psi = gsi_start_phis (gimple_bb (use_stmt)); - + res = true; + gphi_iterator psi = gsi_for_phi (phi); if (scalar_close_phi_node_p (gsi_stmt (psi))) rewrite_close_phi_out_of_ssa (scop, &psi); else
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. Bug introduced by patch: git-svn-id: svn+ssh://gcc.gnu.org/svn/gcc/trunk@217787 --- gcc/graphite-sese-to-poly.c | 7 +++---- 1 file changed, 3 insertions(+), 4 deletions(-)