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

login
register
mail settings
Submitter Richard Guenther
Date Dec. 22, 2010, 12:18 p.m.
Message ID <AANLkTikpbQR2+hSSkR3bn3=xuR7mktbF47d17dbMVNZq@mail.gmail.com>
Download mbox | patch
Permalink /patch/76396/
State New
Headers show

Comments

Richard Guenther - Dec. 22, 2010, 12:18 p.m.
On Wed, Dec 22, 2010 at 4:54 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 21, 2010, Alexandre Oliva <aoliva@redhat.com> wrote:
>
>> The is_gimple_min_invariant() test was a last-minute thought that was
>> supposed to be extended so as to handle the case at hand but not other
>> expressions that can't be moved about, but that I forgot about before
>> testing and posting the patch.  Oops ;-)
>
> Here's a revised version.  Regression-tested on x86_64-linux-gnu after
> bootstrap with BOOT_CFLAGS='-O2 -g -ftree-vectorize'.

Hm.  I don't like the awkward flow of operation in your change.  We
on purpose created debug temps for multiple uses to avoid memory
growth.  So, the if (value_unshare) code in the loop adjusting the
uses should go before that loop and unconditionally create a
debug temporary (and thus doesn't need unsharing either).

In fact - for the case of removed statements we simply should
insert the debug temp at the immediate common dominator of
all uses.  Or without implementing this, can't we avoid this in
the simple-DCE code instead?  Like with


That completely should avoid the names-to-rename issue and I'd be much
more happy with the above for 4.6.

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
>
>
Alexandre Oliva - Dec. 23, 2010, 8:19 a.m.
On Dec 22, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:

> Hm.  I don't like the awkward flow of operation in your change.  We
> on purpose created debug temps for multiple uses to avoid memory
> growth.

I'm not entirely happy with that either, but the savings kind of only
work if we still know where to put the debug temp.  Considering how rare
the need for this fallback is, I figured it didn't make sense to put too
much effort trying to find a common dominator.

In fact, I'm not sure we can count on a common dominator being the right
insertion point, after certain transformations, or even whether we could
figure out whether or not it is the right point after all.  I figured,
when the DEF was already removed and the RHS is movable, inserting just
before the use is simple and conservatively correct, whereas trying to
locate an insertion point at a common dominator would be trickier, more
expensive and ultimately error prone.

Now, maybe we're better off fixing this one instance of the problem,
like you did (just checked with the given testcase), and leaving the
code that attempts to propagates DEFs into DEBUG STMTs alone, so that it
will still crash should other similar situations arise.

Regstrapping your proposed patch now.
Richard Guenther - Dec. 26, 2010, 10:04 p.m.
On Thu, Dec 23, 2010 at 9:19 AM, Alexandre Oliva <aoliva@redhat.com> wrote:
> On Dec 22, 2010, Richard Guenther <richard.guenther@gmail.com> wrote:
>
>> Hm.  I don't like the awkward flow of operation in your change.  We
>> on purpose created debug temps for multiple uses to avoid memory
>> growth.
>
> I'm not entirely happy with that either, but the savings kind of only
> work if we still know where to put the debug temp.  Considering how rare
> the need for this fallback is, I figured it didn't make sense to put too
> much effort trying to find a common dominator.
>
> In fact, I'm not sure we can count on a common dominator being the right
> insertion point, after certain transformations, or even whether we could
> figure out whether or not it is the right point after all.  I figured,
> when the DEF was already removed and the RHS is movable, inserting just
> before the use is simple and conservatively correct, whereas trying to
> locate an insertion point at a common dominator would be trickier, more
> expensive and ultimately error prone.

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

 i_1 = 1;
# i = i_1
 j_2 = i_1 * 2;
# j = j_2
 i_3 = 3;
# i = i_3
 k = j_2;
# k = j_2

and deleting the j = i * 2 statement.  If you propagate into the use
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.

> Now, maybe we're better off fixing this one instance of the problem,
> like you did (just checked with the given testcase), and leaving the
> code that attempts to propagates DEFs into DEBUG STMTs alone, so that it
> will still crash should other similar situations arise.
>
> Regstrapping your proposed patch now.

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

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
>

Patch

Index: tree-vect-loop-manip.c
===================================================================
--- tree-vect-loop-manip.c	(revision 167471)
+++ tree-vect-loop-manip.c	(working copy)
@@ -1442,6 +1442,9 @@ 
   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 @@ 

   adjust_vec_debug_stmts ();

-  BITMAP_FREE (definitions);
-  delete_update_ssa ();
-
   return new_loop;
 }