Re: [PATCH] Restore previous change for gimple_phi_iterator

[Tobias Grosser]

On 07/02/2015 06:52 PM, Aditya Kumar wrote:

gcc/ChangeLog:

2015-07-02  Aditya Kumar  aditya...@samsung.com
Sebastian Pop  s@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Tobias Grosser]

On 07/02/2015 08:34 PM, Sebastian Pop wrote:

On Thu, Jul 2, 2015 at 1:17 PM, Tobias Grosser tob...@grosser.es wrote:

On 07/02/2015 06:52 PM, Aditya Kumar wrote:


gcc/ChangeLog:

2015-07-02  Aditya Kumar  aditya...@samsung.com
 Sebastian Pop  s@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Sebastian Pop]

On Thu, Jul 2, 2015 at 1:44 PM, Tobias Grosser tob...@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Tobias Grosser]

On 07/02/2015 09:03 PM, Sebastian Pop wrote:

On Thu, Jul 2, 2015 at 1:44 PM, Tobias Grosser tob...@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Ramana Radhakrishnan]

On Thu, Jul 2, 2015 at 7:34 PM, Sebastian Pop seb...@gmail.com wrote:
 On Thu, Jul 2, 2015 at 1:17 PM, Tobias Grosser tob...@grosser.es wrote:
 On 07/02/2015 06:52 PM, Aditya Kumar wrote:

 gcc/ChangeLog:

 2015-07-02  Aditya Kumar  aditya...@samsung.com
 Sebastian Pop  s@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Sebastian Pop]

On Thu, Jul 2, 2015 at 2:03 PM, Ramana Radhakrishnan
ramana@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Sebastian Pop]

On Thu, Jul 2, 2015 at 1:17 PM, Tobias Grosser tob...@grosser.es wrote:
 On 07/02/2015 06:52 PM, Aditya Kumar wrote:

 gcc/ChangeLog:

 2015-07-02  Aditya Kumar  aditya...@samsung.com
 Sebastian Pop  s@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

Re: [PATCH] Restore previous change for gimple_phi_iterator

[Tobias Grosser]

On 07/02/2015 09:09 PM, Sebastian Pop wrote:

On Thu, Jul 2, 2015 at 2:03 PM, Ramana Radhakrishnan
ramana@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