| Message ID | orh9k82um5.fsf@livre.home |
|---|---|
| State | New |
| Headers | show |
On Nov 26, 2015, Alexandre Oliva <aoliva@redhat.com> wrote: > for gcc/ChangeLog > PR debug/67355 > * var-tracking.c (reverse_op): Don't add dummy zero to reverse > ops that simplify back to the original value. > * alias.c (refs_newer_value_p): Cut off recursion for > expressions containing the original value. Doh, I forgot an important part of the email. The patch was regstrapped on x86_64-linux-gnu and i686-linux-gnu, so far only in the GCC 5 branch, where both described problems appear. I suppose it might be a bit too late for 5.3, though :-( Is this ok to install in the trunk (assuming regstrap succeeds there too), and in the GCC 5 branch some time after that? (I'm going to be away all week next week, so I'd rather wait till I return to check it in, so that I can address any fallout more promptly)
On Thu, Nov 26, 2015 at 09:45:06PM -0200, Alexandre Oliva wrote: > VTA's cselib expression hashing compares expressions with the same > hash before adding them to the hash table. When there is a collision > involving a self-referencing expression, we could get infinite > recursion, in spite of the cycle breakers already in place. The > problem is currently latent in the trunk, because by chance we don't > get a collision. > > Such value cycles are often introduced by reverse_op; most often, > they're indirect, and then value canonicalization takes care of the > cycle, but if the reverse operation simplifies to the original value, > we used to issue a (plus V (const_int 0)), because at some point > adding a plain value V to a location list as a reverse_op equivalence > caused other problems. > > (Jakub, do you by any chance still remember what those problems were, > some 5+ years ago?) I'm sorry, but I don't remember. Perhaps it has been before some recursion prevention has been added or whatever, maybe your own PR52001? Have you checked if your patch results in any significant debug info quality changes (say on cc1plus itself, using dwlocstat or just comparing .debug_info/.debug_loc sizes)? Jakub
On Nov 27, 2015, Jakub Jelinek <jakub@redhat.com> wrote: > I'm sorry, but I don't remember. Perhaps it has been before some recursion > prevention has been added or whatever, maybe your own PR52001? Yeah. Thanks anyway. > Have you checked if your patch results in any significant debug info quality > changes It might seem a bit Surprising, but there are no differences whatsoever between stage3-*/*.o in bootstraps with or without the patch, except for stage3-gcc/var-tracking.o and stage3-gcc/alias.o, both because of changes to the source code. AFAICT the only significant difference the patch makes is in canonicalization of equivalent values, so that we merge into the same value equivalences coming from both V and (plus V V0) (where V0 is a value known to be (const_int 0). It doesn't immediately affect (plus (plus V V0) V0), though; I think we'd have to break up the two pluses in a debug insn into separate uops in var-tracking to get it to do so, or attempt to simplify the incoming rtl so that both V0s get dropped. As it stands, the values only get merged in the testcase at the later insn that computes the second plus in (plus (plus V V0) V0), at which point the reverse op simplifies to V and then we combine them all. It's precisely when we attempt to determine the VALUE for the reverse op of this second insn that we used to get into infinite recursion. After the patch, we'll have recognized (plus V V0) as equivalent to V during the reverse_op of an earlier insn, between the debug insn and the second plus, and that equivalence cuts off the infinite recursion when checking they're equal for cselib.
On 11/26/2015 04:45 PM, Alexandre Oliva wrote: > VTA's cselib expression hashing compares expressions with the same > hash before adding them to the hash table. When there is a collision > involving a self-referencing expression, we could get infinite > recursion, in spite of the cycle breakers already in place. The > problem is currently latent in the trunk, because by chance we don't > get a collision. > > Such value cycles are often introduced by reverse_op; most often, > they're indirect, and then value canonicalization takes care of the > cycle, but if the reverse operation simplifies to the original value, > we used to issue a (plus V (const_int 0)), because at some point > adding a plain value V to a location list as a reverse_op equivalence > caused other problems. > > (Jakub, do you by any chance still remember what those problems were, > some 5+ years ago?) > > This dummy zero, in turn, caused the value canonicalizer to not fully > realize the equivalence, leading to more complex graphs and, > occasionally, to infinite recursion when comparing such > value-plus-zero expressions recursively. > > Simply using V solves the infinite recursion from the PR testcase, > since the extra equivalence and the preexisting value canonicalization > together prevent recursion while the unrecognized equivalence > wouldn't, but it exposed another infinite recursion in > memrefs_conflict_p: get_addr had a cycle breaker in place, to skip RTL > referencing values introduced after the one we're examining, but it > wouldn't break the cycle if the value itself appeared in the > expression being examined. > > After removing the dummy zero above, this kind of cycle in the > equivalence graph is no longer introduced by VTA itself, but dummy > zeros are also present in generated code, such as in the 32-bit x86's > pro_epilogue_adjust_stack_si_add epilogue insn generated as part of > the builtin longjmp in _Unwind_RaiseException building libgcc's > unwind-dw2.o. So, break the recursion cycle for them too. > > > for gcc/ChangeLog > > PR debug/67355 > * var-tracking.c (reverse_op): Don't add dummy zero to reverse > ops that simplify back to the original value. > * alias.c (refs_newer_value_p): Cut off recursion for > expressions containing the original value. OK. Presumably there's no good way to directly test this, even though we have a good sense of what's happening. Thus no testcase. Insert plug about unit testing here. Jeff
diff --git a/gcc/alias.c b/gcc/alias.c index 9a642dd..d868da3 100644 --- a/gcc/alias.c +++ b/gcc/alias.c @@ -2072,7 +2072,7 @@ base_alias_check (rtx x, rtx x_base, rtx y, rtx y_base, } /* Return TRUE if EXPR refers to a VALUE whose uid is greater than - that of V. */ + (or equal to) that of V. */ static bool refs_newer_value_p (const_rtx expr, rtx v) @@ -2080,7 +2080,7 @@ refs_newer_value_p (const_rtx expr, rtx v) int minuid = CSELIB_VAL_PTR (v)->uid; subrtx_iterator::array_type array; FOR_EACH_SUBRTX (iter, array, expr, NONCONST) - if (GET_CODE (*iter) == VALUE && CSELIB_VAL_PTR (*iter)->uid > minuid) + if (GET_CODE (*iter) == VALUE && CSELIB_VAL_PTR (*iter)->uid >= minuid) return true; return false; } diff --git a/gcc/var-tracking.c b/gcc/var-tracking.c index 9185bfd..07eea84 100644 --- a/gcc/var-tracking.c +++ b/gcc/var-tracking.c @@ -5774,11 +5774,6 @@ reverse_op (rtx val, const_rtx expr, rtx_insn *insn) return; } ret = simplify_gen_binary (code, GET_MODE (val), val, arg); - if (ret == val) - /* Ensure ret isn't VALUE itself (which can happen e.g. for - (plus (reg1) (reg2)) when reg2 is known to be 0), as that - breaks a lot of routines during var-tracking. */ - ret = gen_rtx_fmt_ee (PLUS, GET_MODE (val), val, const0_rtx); break; default: gcc_unreachable ();