PR debug/41229 [VTA] ssa-update former VUSEs-to-be in debug stmts

PR debug/41229 [VTA] ssa-update former VUSEs-to-be in debug stmts

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 975 bytes --]

We don't carry VOPs in debug stmts.  As a result, when an addressable
variable (for which we'd have set a VUSE in a non-debug stmt) becomes
non-addressable because of optimizations, we may fail to rename the
occurrences of the variable in debug stmts, because there won't be any
USEs to be seen.

This patch checks for variables that need renaming in the bound
expressions in debug stmts and, if any are found, it updates the debug
stmt's operands, so that the reference becomes a regular USE, that will
be updated.

I don't like the walking much, but I don't see much of an alternative
short of bringing VOPs into debug stmts, a can of worms I'd rather not
open.  Perhaps an alternative would be to count even addressable DECLs
as regular USEs in debug stmts, but I guess this might require a lot of
special-casing elsewhere.  Perhaps creating a new kind of use for this
purpose would be useful?  Thoughts?

Meanwhile, here's a patch that I'm testing, that fixes this bug.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ssa-update-former-vops-pr41229.patch --]
[-- Type: text/x-diff, Size: 2554 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/41229
	* tree-into-ssa.c (find_symbol_marked_for_renaming): New.
	(prepare_block_for_update): Use it to update debug stmts.

for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/41229
	* gfortran.dg/pr41229.f90: New.

Index: gcc/testsuite/gfortran.dg/pr41229.f90
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gfortran.dg/pr41229.f90	2009-09-03 16:59:29.000000000 -0300
@@ -0,0 +1,10 @@
+! { dg-do compile }
+! { dg-options "-O2 -g" }
+SUBROUTINE cp_fm_triangular_multiply()
+    INTEGER, PARAMETER :: dp=KIND(0.0D0)
+    REAL(dp), ALLOCATABLE, DIMENSION(:)      :: tau, work
+    REAL(KIND=dp), DIMENSION(:, :), POINTER  :: a
+    ndim = SIZE(a,2)
+    ALLOCATE(tau(ndim),STAT=istat)
+    ALLOCATE(work(2*ndim),STAT=istat)
+END SUBROUTINE
Index: gcc/tree-into-ssa.c
===================================================================
--- gcc/tree-into-ssa.c.orig	2009-09-03 16:54:17.000000000 -0300
+++ gcc/tree-into-ssa.c	2009-09-03 17:27:11.000000000 -0300
@@ -2432,6 +2432,28 @@ mark_use_interesting (tree var, gimple s
     }
 }
 
+/* Called via walk_tree, look for DECLs that are marked for renaming.  */
+
+static tree
+find_symbol_marked_for_renaming (tree *tp, int *walk_subtrees, void *data_)
+{
+  struct walk_stmt_info *wi = (struct walk_stmt_info *) data_;
+
+  if (wi->is_lhs)
+    return NULL_TREE;
+
+  if (DECL_P (*tp))
+    {
+      *walk_subtrees = 0;
+      if (symbol_marked_for_renaming (*tp))
+	return *tp;
+    }
+  else if (TREE_CODE (*tp) == SSA_NAME || CONSTANT_CLASS_P (*tp)
+	   || TYPE_P (*tp))
+    *walk_subtrees = 0;
+
+  return NULL_TREE;
+}
 
 /* Do a dominator walk starting at BB processing statements that
    reference symbols in SYMS_TO_RENAME.  This is very similar to
@@ -2493,6 +2515,20 @@ prepare_block_for_update (basic_block bb
       
       stmt = gsi_stmt (si);
 
+      if (gimple_debug_bind_p (stmt))
+	{
+	  struct walk_stmt_info wi;
+
+	  memset (&wi, 0, sizeof (wi));
+
+	  /* If there's any referenced symbol that is to be put in SSA
+	     form, add it to the list of USEs so that it is marked and
+	     updated.  We need to do this for debug stmts because they
+	     don't have virtual ops.  */
+	  if (walk_gimple_op (stmt, find_symbol_marked_for_renaming, &wi))
+	    update_stmt (stmt);
+	}
+
       FOR_EACH_SSA_USE_OPERAND (use_p, stmt, i, SSA_OP_ALL_USES)
 	{
 	  tree use = USE_FROM_PTR (use_p);

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: PR debug/41229 [VTA] ssa-update former VUSEs-to-be in debug stmts

[Richard Guenther]

On Thu, Sep 3, 2009 at 10:47 PM, Alexandre Oliva<aoliva@redhat.com> wrote:
> We don't carry VOPs in debug stmts.  As a result, when an addressable
> variable (for which we'd have set a VUSE in a non-debug stmt) becomes
> non-addressable because of optimizations, we may fail to rename the
> occurrences of the variable in debug stmts, because there won't be any
> USEs to be seen.
>
> This patch checks for variables that need renaming in the bound
> expressions in debug stmts and, if any are found, it updates the debug
> stmt's operands, so that the reference becomes a regular USE, that will
> be updated.
>
> I don't like the walking much, but I don't see much of an alternative
> short of bringing VOPs into debug stmts, a can of worms I'd rather not
> open.  Perhaps an alternative would be to count even addressable DECLs
> as regular USEs in debug stmts, but I guess this might require a lot of
> special-casing elsewhere.  Perhaps creating a new kind of use for this
> purpose would be useful?  Thoughts?

What does this have to do with VOPs?  On the trunk we'd have

  # .MEM_2 = VUSE <.MEM_1(D)>
  i = 1;
  j_3 = i;
  # DEBUG j => j_3

and now we DCE j_3 and get

  # .MEM_2 = VUSE <.MEM_1(D)>
  i = 1;
  # DEBUG j => i

IIRC, right?  Now suppose i is no longer addressable and we mark i
for rewriting into SSA form.  That's your scenario, no?

I don't see how VOPs for the debug stmts would help at all here.

The key is that execute_update_addresses_taken updates all statements
that have uses of the symbol and the operand scanner adds the bare
symbols as regular uses.  Thus the proper place for the fix is in
execute_update_addresses_taken, the final loop conditional on
update_vops (what a bad name now ...) that updates all stmts
referencing memory (overkill of course).  Just add to the overkill and
update all debug stmts unconditionally.

> Meanwhile, here's a patch that I'm testing, that fixes this bug.

A patch that adds

   if (gimple_debug_bind_p (stmt))
     update_stmt (stmt);

with a comment in the referenced place is ok.

Thanks,
Richard.

>
>
> --
> Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
> You must be the change you wish to see in the world. -- Gandhi
> Be Free! -- http://FSFLA.org/   FSF Latin America board member
> Free Software Evangelist      Red Hat Brazil Compiler Engineer
>
>

Re: PR debug/41229 [VTA] ssa-update former VUSEs-to-be in debug stmts

[Alexandre Oliva]

On Sep  4, 2009, Richard Guenther <richard.guenther@gmail.com> wrote:

> On Thu, Sep 3, 2009 at 10:47 PM, Alexandre Oliva<aoliva@redhat.com> wrote:
>> This patch checks for variables that need renaming in the bound
>> expressions in debug stmts and, if any are found, it updates the debug
>> stmt's operands, so that the reference becomes a regular USE, that will
>> be updated.

> What does this have to do with VOPs?

Maybe nothing.  I assumed the VUSEs could be used to locate and adjust
the affected variables, limiting the effect only to relevant statements.
If that's not so, then clearly VOPs for debug insns won't help.

>   # .MEM_2 = VUSE <.MEM_1(D)>
>   i = 1;
>   # DEBUG j => i

> IIRC, right?  Now suppose i is no longer addressable and we mark i
> for rewriting into SSA form.  That's your scenario, no?

Exactly.

> The key is that execute_update_addresses_taken updates all statements
> that have uses of the symbol and the operand scanner adds the bare
> symbols as regular uses.  Thus the proper place for the fix is in
> execute_update_addresses_taken, the final loop conditional on
> update_vops (what a bad name now ...) that updates all stmts
> referencing memory (overkill of course).  Just add to the overkill and
> update all debug stmts unconditionally.

I'm a bit concerned about updating all debug stmts, because this might
force some blocks to be updated that wouldn't be updated otherwise, and
this may lead to codegen differences.  But I guess at least updating the
stmt is unavoidable, so I'll do that, and if there's fallout, I'll deal
with it.

Thanks for the suggestion, I'll implement the pre-approved patch, give
it a round of testing, and then post it and check it in if it passes.

-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer

Re: PR debug/41229 [VTA] ssa-update former VUSEs-to-be in debug stmts

[Alexandre Oliva]

[-- Attachment #1: Type: text/plain, Size: 569 bytes --]

On Sep  4, 2009, Alexandre Oliva <aoliva@redhat.com> wrote:

> I'm a bit concerned about updating all debug stmts, because this might
> force some blocks to be updated that wouldn't be updated otherwise, and
> this may lead to codegen differences.  But I guess at least updating the
> stmt is unavoidable, so I'll do that, and if there's fallout, I'll deal
> with it.

> Thanks for the suggestion, I'll implement the pre-approved patch, give
> it a round of testing, and then post it and check it in if it passes.

It worked.  Here's what I'm checking in momentarily.


[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #2: vta-ssa-update-former-vops-pr41229.patch --]
[-- Type: text/x-diff, Size: 1356 bytes --]

for  gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/41229
	* tree-ssa.c (execute_update_addresses_taken): Update debug insns.

for  gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR debug/41229
	* gfortran.dg/pr41229.f90: New.

Index: gcc/testsuite/gfortran.dg/pr41229.f90
===================================================================
--- /dev/null	1970-01-01 00:00:00.000000000 +0000
+++ gcc/testsuite/gfortran.dg/pr41229.f90	2009-09-04 04:10:11.000000000 -0300
@@ -0,0 +1,10 @@
+! { dg-do compile }
+! { dg-options "-O2 -g" }
+SUBROUTINE cp_fm_triangular_multiply()
+    INTEGER, PARAMETER :: dp=KIND(0.0D0)
+    REAL(dp), ALLOCATABLE, DIMENSION(:)      :: tau, work
+    REAL(KIND=dp), DIMENSION(:, :), POINTER  :: a
+    ndim = SIZE(a,2)
+    ALLOCATE(tau(ndim),STAT=istat)
+    ALLOCATE(work(2*ndim),STAT=istat)
+END SUBROUTINE
Index: gcc/tree-ssa.c
===================================================================
--- gcc/tree-ssa.c.orig	2009-09-04 15:39:33.000000000 -0300
+++ gcc/tree-ssa.c	2009-09-04 15:41:57.000000000 -0300
@@ -1893,7 +1893,8 @@ execute_update_addresses_taken (bool do_
 	    {
 	      gimple stmt = gsi_stmt (gsi);
 
-	      if (gimple_references_memory_p (stmt))
+	      if (gimple_references_memory_p (stmt)
+		  || is_gimple_debug (stmt))
 		update_stmt (stmt);
 	    }
 

[-- Attachment #3: Type: text/plain, Size: 257 bytes --]


-- 
Alexandre Oliva, freedom fighter    http://FSFLA.org/~lxoliva/
You must be the change you wish to see in the world. -- Gandhi
Be Free! -- http://FSFLA.org/   FSF Latin America board member
Free Software Evangelist      Red Hat Brazil Compiler Engineer