Fix PR47612

Fix PR47612

[Bernd Schmidt]

[-- Attachment #1: Type: text/plain, Size: 395 bytes --]

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.


Bernd

[-- Attachment #2: dfp2.diff --]
[-- Type: text/plain, Size: 939 bytes --]

	* df-problems.c (can_move_insns_across): Don't pick a cc0 setter
	as the last insn of the sequence to be moved.

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;

Re: Fix PR47612

[Michael Matz]

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.

Re: Fix PR47612

[Bernd Schmidt]

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

Re: Fix PR47612

[Michael Matz]

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.