Patchwork [PR,debug/46931] don't crash propagating removed DEFs into debug stmts

login
register
mail settings
Submitter Alexandre Oliva
Date Dec. 28, 2010, 8:26 p.m.
Message ID <orhbdxwsnr.fsf@livre.localdomain>
Download mbox | patch
Permalink /patch/76864/
State New
Headers show

Comments

Alexandre Oliva - Dec. 28, 2010, 8:26 p.m.
On Dec 26, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> Yeah.  But wasn't there a correctness problem with propagating as well?

> the the debug info will refer to the wrong value of i.  Remember that
> we can have overlapping life ranges for SSA names but not for the
> decls for which we emit debug info.

The value expressions in debug stmts refer to the SSA names, so there's
no problem, at least in this regard.  The overlapping ranges, if
expanded to different pseudos, it will get the correct RTL expressions;
if no longer available at a point, it should not get any RTL expression
(although it would in theory be possible to do better, looking for some
equivalence).

Now, I don't think the latter has been actually verified, especially
after the change that made us go straight from SSA to RTL, so if you
have evidence that we're doing it wrong, I'd love to see it.

>> Regstrapping your proposed patch now.

> It's ok to commit if it works (with your testcase).

Here's what I installed.
Richard Guenther - Dec. 28, 2010, 8:43 p.m.
On Tue, Dec 28, 2010 at 9:26 PM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 26, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Yeah.  But wasn't there a correctness problem with propagating as well?
>
>> the the debug info will refer to the wrong value of i.  Remember that
>> we can have overlapping life ranges for SSA names but not for the
>> decls for which we emit debug info.
>
> The value expressions in debug stmts refer to the SSA names, so there's
> no problem, at least in this regard.  The overlapping ranges, if
> expanded to different pseudos, it will get the correct RTL expressions;
> if no longer available at a point, it should not get any RTL expression
> (although it would in theory be possible to do better, looking for some
> equivalence).
>
> Now, I don't think the latter has been actually verified, especially
> after the change that made us go straight from SSA to RTL, so if you
> have evidence that we're doing it wrong, I'd love to see it.

I don't have evidence, it's just what I seem to remember (for some
reason).  Btw, we can have memory operands on the RHS of debug
stmts?  For those propagating wouldn't be valid as they are not
in SSA form.

Richard.

>>> Regstrapping your proposed patch now.
>
>> It's ok to commit if it works (with your testcase).
>
> Here's what I installed.
>
>
>
> --
> 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
>
>
Alexandre Oliva - Dec. 29, 2010, 5:30 a.m.
On Dec 28, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> we can have memory operands on the RHS of debug stmts?  For those
> propagating wouldn't be valid as they are not in SSA form.

Yup, the code I posted wouldn't propagate MEMs.

Patch

for  gcc/ChangeLog
from  Richard Guenther  <rguenther@suse.de>

	PR debug/46931
	* tree-vect-loop-manip.c (slpeel_tree_peel_loop_to_edge): Update
	SSA before removing dead stmts.

Index: gcc/tree-vect-loop-manip.c
===================================================================
--- gcc/tree-vect-loop-manip.c.orig	2010-12-18 04:28:14.000000000 -0200
+++ gcc/tree-vect-loop-manip.c	2010-12-23 05:50:15.540018010 -0200
@@ -1442,6 +1442,9 @@  slpeel_tree_peel_loop_to_edge (struct lo
   if (update_first_loop_count)
     slpeel_make_loop_iterate_ntimes (first_loop, first_niters);
 
+  BITMAP_FREE (definitions);
+  delete_update_ssa ();
+
   /* Remove all pattern statements from the loop copy.  They will confuse
      the expander if DCE is disabled.
      ???  The pattern recognizer should be split into an analysis and
@@ -1451,9 +1454,6 @@  slpeel_tree_peel_loop_to_edge (struct lo
 
   adjust_vec_debug_stmts ();
 
-  BITMAP_FREE (definitions);
-  delete_update_ssa ();
-
   return new_loop;
 }