| Message ID | AANLkTikpbQR2+hSSkR3bn3=xuR7mktbF47d17dbMVNZq@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
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.
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 >
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; }