| Message ID | 4DA4B67C.6020600@codesourcery.com |
|---|---|
| State | New |
| Headers | show |
Hi, On Tue, 12 Apr 2011, Bernd Schmidt wrote: > This fixes a problem on cc0 machines where we split a sequence of insns > at a point where we shouldn't - between a cc0 setter and a cc0 user. > > The fix is simple enough; just make sure not to pick a cc0 setter as the > end of such a sequence. The patch below was regression tested on > m68k-rtems4.11 by Joel Sherrill; I'll commit it as obvious in a few days > unless someone tells me it isn't. I'm a big fan of commentary. I'm an even larger fan of two-to-three sentence commentary for just a single conditional skip. Even more so if it's related to cc0 targets. And is there any chance to transform this: +#ifdef HAVE_cc0 + if (!sets_cc0_p (insn)) +#endif + max_to = insn; into this: + if (!sets_cc0_p (insn)) max_to = insn; ? Yes, that implies making sets_cc0_p be always there and return false, and write the conditionals in the corrent way? I'll also note that the first hunk of your change is in a loop commented with "Compute upper bound, bla ...", meaning to be a heuristic, and your second change is this: - if (!bitmap_intersect_p (test_set, local_merge_live)) + if (!bitmap_intersect_p (test_set, local_merge_live) +#ifdef HAVE_cc0 + && !sets_cc0_p (insn) +#endif + ) It seems to me, that those insn then shouldn't have been in test_set from the start, instead of fiddling with the users of test_set. Hence, is my feeling of the patch being a hack-around of a deeper problem or it being the wrong place to hack wrong? Ciao, Michael.
On 04/13/2011 01:45 AM, Michael Matz wrote: > And is there any chance to transform this: > > +#ifdef HAVE_cc0 > + if (!sets_cc0_p (insn)) > +#endif > + max_to = insn; > > into this: > > + if (!sets_cc0_p (insn)) > max_to = insn; > > ? Yes, that implies making sets_cc0_p be always there and return false, > and write the conditionals in the corrent way? The correct fix is to eliminate cc0 targets. > I'll also note that the first hunk of your change is in a loop commented > with "Compute upper bound, bla ...", meaning to be a heuristic, and your > second change is this: It's not a heuristic. Both of these loops are necessary to compute a valid end point. !sets_cc0_p is just an additional condition that must be satisfied in both of them. > - if (!bitmap_intersect_p (test_set, local_merge_live)) > + if (!bitmap_intersect_p (test_set, local_merge_live) > +#ifdef HAVE_cc0 > + && !sets_cc0_p (insn) > +#endif > + ) > > It seems to me, that those insn then shouldn't have been in test_set from > the start, instead of fiddling with the users of test_set. Hence, is my > feeling of the patch being a hack-around of a deeper problem or it being > the wrong place to hack wrong? I don't understand this sentence. Bernd
Hi, On Wed, 13 Apr 2011, Bernd Schmidt wrote: > > I'll also note that the first hunk of your change is in a loop commented > > with "Compute upper bound, bla ...", meaning to be a heuristic, and your > > second change is this: > > It's not a heuristic. Both of these loops are necessary to compute a > valid end point. !sets_cc0_p is just an additional condition that must > be satisfied in both of them. Yeah, sorry, I haven't read the whole context in that routine, and misread some thing, causing ... > > +#ifdef HAVE_cc0 > > + && !sets_cc0_p (insn) > > +#endif > > + ) > > > > It seems to me, that those insn then shouldn't have been in test_set from > > the start, instead of fiddling with the users of test_set. Hence, is my > > feeling of the patch being a hack-around of a deeper problem or it being > > the wrong place to hack wrong? > > I don't understand this sentence. ... this. I shouldn't look at patches when I'm only half-awake. Ciao, Michael.
Index: df-problems.c =================================================================== --- df-problems.c (revision 172094) +++ df-problems.c (working copy) @@ -4001,7 +4001,10 @@ can_move_insns_across (rtx from, rtx to, if (bitmap_intersect_p (merge_set, test_use) || bitmap_intersect_p (merge_use, test_set)) break; - max_to = insn; +#ifdef HAVE_cc0 + if (!sets_cc0_p (insn)) +#endif + max_to = insn; } next = NEXT_INSN (insn); if (insn == to) @@ -4038,7 +4041,11 @@ can_move_insns_across (rtx from, rtx to, { if (NONDEBUG_INSN_P (insn)) { - if (!bitmap_intersect_p (test_set, local_merge_live)) + if (!bitmap_intersect_p (test_set, local_merge_live) +#ifdef HAVE_cc0 + && !sets_cc0_p (insn) +#endif + ) { max_to = insn; break;