diff mbox

[2/2] refactor emit_*_{after,before}{,_setloc} using common functions

Message ID 1300763907-15774-3-git-send-email-froydnj@codesourcery.com
State New
Headers show

Commit Message

Nathan Froyd March 22, 2011, 3:18 a.m. UTC 
This patch builds on the previous one to refactor the close cousins of
the *_noloc family.  I attempted to separate these out into separate
patches, one dealing with *_setloc and the other dealing with the
remainder, but I did not trust myself to do it correctly even with
magit's help.

	* emit-rtl.c (emit_pattern_after_setloc): New function.
	(emit_insn_after_setloc, emit_jump_insn_after_setloc): Call it.
	(emit_call_insn_after_setloc, emit_debug_insn_after_setloc): Likewise.
	(emit_pattern_after): New function.
	(emit_insn_after, emit_jump_insn_after): Call it.
	(emit_call_insn_after, emit_debug_insn_after): Likewise.
	(emit_pattern_before_setloc): New function.
	(emit_insn_before_setloc, emit_jump_insn_before_setloc): Call it.
	(emit_call_insn_before_setloc, emit_debug_insn_before_setloc):
	Likewise.
	(emit_pattern_before): New function.
	(emit_insn_before, emit_jump_insn_before): Call it.
	(emit_call_insn_before, emit_debug_insn_before): Likewise.

Comments

Richard Henderson March 22, 2011, 9:28 p.m. UTC | #1 
On 03/21/2011 08:18 PM, Nathan Froyd wrote:
> +/* Insert PATTERN before BEFORE, setting its INSN_LOCATION to LOC.
> +   MAKE_RAW indicates how to turn PATTERN into a real insn.  INSNP
> +   indicates if PATTERN is meant for an INSN as opposed to a JUMP_INSN,
> +   CALL_INSN, etc.  */
> +
> +static rtx
> +emit_pattern_before_setloc (rtx pattern, rtx before, int loc, bool insnp,
> +			    rtx (*make_raw) (rtx))
>  {
>    rtx first = PREV_INSN (before);
> -  rtx last = emit_insn_before_noloc (pattern, before, NULL);
> +  rtx last = emit_pattern_before_noloc (pattern, before,
> +                                        insnp ? before : NULL_RTX,
> +                                        NULL, make_raw);
>  
> -  if (pattern == NULL_RTX || !loc)
> +  if (pattern == NULL_RTX || (!loc && insnp))
>      return last;
>  
> -  if (!first)
> +  if (!first && insnp)
>      first = get_insns ();

Is this not merely indicative of a bug in one or more of the functions?
Why would recording LOC be dependent on INSNP?


r~
Nathan Froyd March 22, 2011, 11:13 p.m. UTC | #2 
On Tue, Mar 22, 2011 at 02:28:30PM -0700, Richard Henderson wrote:
> On 03/21/2011 08:18 PM, Nathan Froyd wrote:
> > +/* Insert PATTERN before BEFORE, setting its INSN_LOCATION to LOC.
> > +   MAKE_RAW indicates how to turn PATTERN into a real insn.  INSNP
> > +   indicates if PATTERN is meant for an INSN as opposed to a JUMP_INSN,
> > +   CALL_INSN, etc.  */
> > +
> > +static rtx
> > +emit_pattern_before_setloc (rtx pattern, rtx before, int loc, bool insnp,
> > +			    rtx (*make_raw) (rtx))
> >  {
> >    rtx first = PREV_INSN (before);
> > -  rtx last = emit_insn_before_noloc (pattern, before, NULL);
> > +  rtx last = emit_pattern_before_noloc (pattern, before,
> > +                                        insnp ? before : NULL_RTX,
> > +                                        NULL, make_raw);
> >  
> > -  if (pattern == NULL_RTX || !loc)
> > +  if (pattern == NULL_RTX || (!loc && insnp))
> >      return last;
> >  
> > -  if (!first)
> > +  if (!first && insnp)
> >      first = get_insns ();
> 
> Is this not merely indicative of a bug in one or more of the functions?
> Why would recording LOC be dependent on INSNP?

That's a good question; I don't know.  I was trying to preserve the
existing behavior.  Compare:

rtx
emit_insn_before_setloc (rtx pattern, rtx before, int loc)
{
  ...
  if (pattern == NULL_RTX || !loc)
    return last;

  if (!first)
    first = get_insns ();
  else
    first = NEXT_INSN (first);
  ...

to the similar:

rtx
emit_call_insn_before_setloc (rtx pattern, rtx before, int loc)
{
  ...
  if (pattern == NULL_RTX)
    return last;

  first = NEXT_INSN (first);

The jump_insn and debug_insn variants have identical behavior to
call_insn.

-Nathan
Richard Henderson March 23, 2011, 12:06 a.m. UTC | #3 
On 03/22/2011 04:13 PM, Nathan Froyd wrote:
> rtx
> emit_call_insn_before_setloc (rtx pattern, rtx before, int loc)
> {
>   ...
>   if (pattern == NULL_RTX)
>     return last;
> 
>   first = NEXT_INSN (first);
> 
> The jump_insn and debug_insn variants have identical behavior to
> call_insn.

AFAICT, the !loc test really ought to be loc != UNKNOWN_LOCATION,
and ought to apply to all of the patterns.  This makes sense once
you look at the test written canonically -- no point in searching
for the list of insns if there's no location to set.

Further, the !first test can be applied everywhere.  This handles
the edge condition of emitting an insn as the very first in a 
sequence.  Something that's unlikely to happen for calls and jumps,
but needn't be special cased away.

Ok with those changes, assuming they bootstrap.



r~
Nathan Froyd March 23, 2011, 12:09 p.m. UTC | #4 
On Tue, Mar 22, 2011 at 05:06:39PM -0700, Richard Henderson wrote:
> On 03/22/2011 04:13 PM, Nathan Froyd wrote:
> > rtx
> > emit_call_insn_before_setloc (rtx pattern, rtx before, int loc)
> > {
> >   ...
> >   if (pattern == NULL_RTX)
> >     return last;
> > 
> >   first = NEXT_INSN (first);
> > 
> > The jump_insn and debug_insn variants have identical behavior to
> > call_insn.
> 
> AFAICT, the !loc test really ought to be loc != UNKNOWN_LOCATION,
> and ought to apply to all of the patterns.  This makes sense once
> you look at the test written canonically -- no point in searching
> for the list of insns if there's no location to set.

Did you mean loc == UNKNOWN_LOCATION?  Also, it looks like that's
conflating INSN_LOCATORs and location_ts; it seems like it'd be better
to keep them separate.

-Nathan
Richard Henderson March 23, 2011, 6:36 p.m. UTC | #5 
On 03/23/2011 05:09 AM, Nathan Froyd wrote:
> Did you mean loc == UNKNOWN_LOCATION?  Also, it looks like that's
> conflating INSN_LOCATORs and location_ts; it seems like it'd be better
> to keep them separate.

Ug.  Yes and yes.  I'd forgotten that insn_locators are different
from line locations.  And indeed, there's no magic define for the
no-insn-locator condition; it's just a literal zero.

But still, there's no reason not to apply the same zero optimization
for all insn types.


r~
diff mbox

Patch

diff --git a/gcc/emit-rtl.c b/gcc/emit-rtl.c
index 9b6f0f8..aabdc73 100644
--- a/gcc/emit-rtl.c
+++ b/gcc/emit-rtl.c
@@ -4316,11 +4316,14 @@  emit_note_after (enum insn_note subtype, rtx after)
   return note;
 }
 
-/* Like emit_insn_after_noloc, but set INSN_LOCATOR according to SCOPE.  */
-rtx
-emit_insn_after_setloc (rtx pattern, rtx after, int loc)
+/* Insert PATTERN after AFTER, setting its INSN_LOCATION to LOC.
+   MAKE_RAW indicates how to turn PATTERN into a real insn.  */
+
+static rtx
+emit_pattern_after_setloc (rtx pattern, rtx after, int loc,
+			   rtx (*make_raw) (rtx))
 {
-  rtx last = emit_insn_after_noloc (pattern, after, NULL);
+  rtx last = emit_pattern_after_noloc (pattern, after, NULL, make_raw);
 
   if (pattern == NULL_RTX || !loc)
     return last;
@@ -4337,135 +4340,101 @@  emit_insn_after_setloc (rtx pattern, rtx after, int loc)
   return last;
 }
 
-/* Like emit_insn_after_noloc, but set INSN_LOCATOR according to AFTER.  */
-rtx
-emit_insn_after (rtx pattern, rtx after)
+/* Insert PATTERN after AFTER.  MAKE_RAW indicates how to turn PATTERN
+   into a real insn.  SKIP_DEBUG_INSNS indicates whether to insert after
+   any DEBUG_INSNs.  */
+
+static rtx
+emit_pattern_after (rtx pattern, rtx after, bool skip_debug_insns,
+		    rtx (*make_raw) (rtx))
 {
   rtx prev = after;
 
-  while (DEBUG_INSN_P (prev))
-    prev = PREV_INSN (prev);
+  if (skip_debug_insns)
+    while (DEBUG_INSN_P (prev))
+      prev = PREV_INSN (prev);
 
   if (INSN_P (prev))
-    return emit_insn_after_setloc (pattern, after, INSN_LOCATOR (prev));
+    return emit_pattern_after_setloc (pattern, after, INSN_LOCATOR (prev),
+				      make_raw);
   else
-    return emit_insn_after_noloc (pattern, after, NULL);
+    return emit_pattern_after_noloc (pattern, after, NULL, make_raw);
 }
 
-/* Like emit_jump_insn_after_noloc, but set INSN_LOCATOR according to SCOPE.  */
+/* Like emit_insn_after_noloc, but set INSN_LOCATOR according to LOC.  */
 rtx
-emit_jump_insn_after_setloc (rtx pattern, rtx after, int loc)
+emit_insn_after_setloc (rtx pattern, rtx after, int loc)
 {
-  rtx last = emit_jump_insn_after_noloc (pattern, after);
+  return emit_pattern_after_setloc (pattern, after, loc, make_insn_raw);
+}
 
-  if (pattern == NULL_RTX || !loc)
-    return last;
+/* Like emit_insn_after_noloc, but set INSN_LOCATOR according to AFTER.  */
+rtx
+emit_insn_after (rtx pattern, rtx after)
+{
+  return emit_pattern_after (pattern, after, true, make_insn_raw);
+}
 
-  after = NEXT_INSN (after);
-  while (1)
-    {
-      if (active_insn_p (after) && !INSN_LOCATOR (after))
-	INSN_LOCATOR (after) = loc;
-      if (after == last)
-	break;
-      after = NEXT_INSN (after);
-    }
-  return last;
+/* Like emit_jump_insn_after_noloc, but set INSN_LOCATOR according to LOC.  */
+rtx
+emit_jump_insn_after_setloc (rtx pattern, rtx after, int loc)
+{
+  return emit_pattern_after_setloc (pattern, after, loc, make_jump_insn_raw);
 }
 
 /* Like emit_jump_insn_after_noloc, but set INSN_LOCATOR according to AFTER.  */
 rtx
 emit_jump_insn_after (rtx pattern, rtx after)
 {
-  rtx prev = after;
-
-  while (DEBUG_INSN_P (prev))
-    prev = PREV_INSN (prev);
-
-  if (INSN_P (prev))
-    return emit_jump_insn_after_setloc (pattern, after, INSN_LOCATOR (prev));
-  else
-    return emit_jump_insn_after_noloc (pattern, after);
+  return emit_pattern_after (pattern, after, true, make_jump_insn_raw);
 }
 
-/* Like emit_call_insn_after_noloc, but set INSN_LOCATOR according to SCOPE.  */
+/* Like emit_call_insn_after_noloc, but set INSN_LOCATOR according to LOC.  */
 rtx
 emit_call_insn_after_setloc (rtx pattern, rtx after, int loc)
 {
-  rtx last = emit_call_insn_after_noloc (pattern, after);
-
-  if (pattern == NULL_RTX || !loc)
-    return last;
-
-  after = NEXT_INSN (after);
-  while (1)
-    {
-      if (active_insn_p (after) && !INSN_LOCATOR (after))
-	INSN_LOCATOR (after) = loc;
-      if (after == last)
-	break;
-      after = NEXT_INSN (after);
-    }
-  return last;
+  return emit_pattern_after_setloc (pattern, after, loc, make_call_insn_raw);
 }
 
 /* Like emit_call_insn_after_noloc, but set INSN_LOCATOR according to AFTER.  */
 rtx
 emit_call_insn_after (rtx pattern, rtx after)
 {
-  rtx prev = after;
-
-  while (DEBUG_INSN_P (prev))
-    prev = PREV_INSN (prev);
-
-  if (INSN_P (prev))
-    return emit_call_insn_after_setloc (pattern, after, INSN_LOCATOR (prev));
-  else
-    return emit_call_insn_after_noloc (pattern, after);
+  return emit_pattern_after (pattern, after, true, make_call_insn_raw);
 }
 
-/* Like emit_debug_insn_after_noloc, but set INSN_LOCATOR according to SCOPE.  */
+/* Like emit_debug_insn_after_noloc, but set INSN_LOCATOR according to LOC.  */
 rtx
 emit_debug_insn_after_setloc (rtx pattern, rtx after, int loc)
 {
-  rtx last = emit_debug_insn_after_noloc (pattern, after);
-
-  if (pattern == NULL_RTX || !loc)
-    return last;
-
-  after = NEXT_INSN (after);
-  while (1)
-    {
-      if (active_insn_p (after) && !INSN_LOCATOR (after))
-	INSN_LOCATOR (after) = loc;
-      if (after == last)
-	break;
-      after = NEXT_INSN (after);
-    }
-  return last;
+  return emit_pattern_after_setloc (pattern, after, loc, make_debug_insn_raw);
 }
 
 /* Like emit_debug_insn_after_noloc, but set INSN_LOCATOR according to AFTER.  */
 rtx
 emit_debug_insn_after (rtx pattern, rtx after)
 {
-  if (INSN_P (after))
-    return emit_debug_insn_after_setloc (pattern, after, INSN_LOCATOR (after));
-  else
-    return emit_debug_insn_after_noloc (pattern, after);
+  return emit_pattern_after (pattern, after, false, make_debug_insn_raw);
 }
 
-/* Like emit_insn_before_noloc, but set INSN_LOCATOR according to SCOPE.  */
-rtx
-emit_insn_before_setloc (rtx pattern, rtx before, int loc)
+/* Insert PATTERN before BEFORE, setting its INSN_LOCATION to LOC.
+   MAKE_RAW indicates how to turn PATTERN into a real insn.  INSNP
+   indicates if PATTERN is meant for an INSN as opposed to a JUMP_INSN,
+   CALL_INSN, etc.  */
+
+static rtx
+emit_pattern_before_setloc (rtx pattern, rtx before, int loc, bool insnp,
+			    rtx (*make_raw) (rtx))
 {
   rtx first = PREV_INSN (before);
-  rtx last = emit_insn_before_noloc (pattern, before, NULL);
+  rtx last = emit_pattern_before_noloc (pattern, before,
+                                        insnp ? before : NULL_RTX,
+                                        NULL, make_raw);
 
-  if (pattern == NULL_RTX || !loc)
+  if (pattern == NULL_RTX || (!loc && insnp))
     return last;
 
-  if (!first)
+  if (!first && insnp)
     first = get_insns ();
   else
     first = NEXT_INSN (first);
@@ -4480,127 +4449,93 @@  emit_insn_before_setloc (rtx pattern, rtx before, int loc)
   return last;
 }
 
-/* Like emit_insn_before_noloc, but set INSN_LOCATOR according to BEFORE.  */
-rtx
-emit_insn_before (rtx pattern, rtx before)
+/* Insert PATTERN before BEFORE.  MAKE_RAW indicates how to turn PATTERN
+   into a real insn.  SKIP_DEBUG_INSNS indicates whether to insert
+   before any DEBUG_INSNs.  INSNP indicates if PATTERN is meant for an
+   INSN as opposed to a JUMP_INSN, CALL_INSN, etc.  */
+
+static rtx
+emit_pattern_before (rtx pattern, rtx before, bool skip_debug_insns,
+		     bool insnp, rtx (*make_raw) (rtx))
 {
   rtx next = before;
 
-  while (DEBUG_INSN_P (next))
-    next = PREV_INSN (next);
+  if (skip_debug_insns)
+    while (DEBUG_INSN_P (next))
+      next = PREV_INSN (next);
 
   if (INSN_P (next))
-    return emit_insn_before_setloc (pattern, before, INSN_LOCATOR (next));
+    return emit_pattern_before_setloc (pattern, before, INSN_LOCATOR (next),
+				       insnp, make_raw);
   else
-    return emit_insn_before_noloc (pattern, before, NULL);
+    return emit_pattern_before_noloc (pattern, before,
+                                      insnp ? before : NULL_RTX,
+                                      NULL, make_raw);
 }
 
-/* like emit_insn_before_noloc, but set insn_locator according to scope.  */
+/* Like emit_insn_before_noloc, but set INSN_LOCATOR according to LOC.  */
 rtx
-emit_jump_insn_before_setloc (rtx pattern, rtx before, int loc)
+emit_insn_before_setloc (rtx pattern, rtx before, int loc)
 {
-  rtx first = PREV_INSN (before);
-  rtx last = emit_jump_insn_before_noloc (pattern, before);
+  return emit_pattern_before_setloc (pattern, before, loc, true,
+				     make_insn_raw);
+}
 
-  if (pattern == NULL_RTX)
-    return last;
+/* Like emit_insn_before_noloc, but set INSN_LOCATOR according to BEFORE.  */
+rtx
+emit_insn_before (rtx pattern, rtx before)
+{
+  return emit_pattern_before (pattern, before, true, true, make_insn_raw);
+}
 
-  first = NEXT_INSN (first);
-  while (1)
-    {
-      if (active_insn_p (first) && !INSN_LOCATOR (first))
-	INSN_LOCATOR (first) = loc;
-      if (first == last)
-	break;
-      first = NEXT_INSN (first);
-    }
-  return last;
+/* like emit_insn_before_noloc, but set INSN_LOCATOR according to LOC.  */
+rtx
+emit_jump_insn_before_setloc (rtx pattern, rtx before, int loc)
+{
+  return emit_pattern_before_setloc (pattern, before, loc, false,
+				     make_jump_insn_raw);
 }
 
 /* Like emit_jump_insn_before_noloc, but set INSN_LOCATOR according to BEFORE.  */
 rtx
 emit_jump_insn_before (rtx pattern, rtx before)
 {
-  rtx next = before;
-
-  while (DEBUG_INSN_P (next))
-    next = PREV_INSN (next);
-
-  if (INSN_P (next))
-    return emit_jump_insn_before_setloc (pattern, before, INSN_LOCATOR (next));
-  else
-    return emit_jump_insn_before_noloc (pattern, before);
+  return emit_pattern_before (pattern, before, true, false,
+			      make_jump_insn_raw);
 }
 
-/* like emit_insn_before_noloc, but set insn_locator according to scope.  */
+/* Like emit_insn_before_noloc, but set INSN_LOCATOR according to LOC.  */
 rtx
 emit_call_insn_before_setloc (rtx pattern, rtx before, int loc)
 {
-  rtx first = PREV_INSN (before);
-  rtx last = emit_call_insn_before_noloc (pattern, before);
-
-  if (pattern == NULL_RTX)
-    return last;
-
-  first = NEXT_INSN (first);
-  while (1)
-    {
-      if (active_insn_p (first) && !INSN_LOCATOR (first))
-	INSN_LOCATOR (first) = loc;
-      if (first == last)
-	break;
-      first = NEXT_INSN (first);
-    }
-  return last;
+  return emit_pattern_before_setloc (pattern, before, loc, false,
+				     make_call_insn_raw);
 }
 
-/* like emit_call_insn_before_noloc,
-   but set insn_locator according to before.  */
+/* Like emit_call_insn_before_noloc,
+   but set insn_locator according to BEFORE.  */
 rtx
 emit_call_insn_before (rtx pattern, rtx before)
 {
-  rtx next = before;
-
-  while (DEBUG_INSN_P (next))
-    next = PREV_INSN (next);
-
-  if (INSN_P (next))
-    return emit_call_insn_before_setloc (pattern, before, INSN_LOCATOR (next));
-  else
-    return emit_call_insn_before_noloc (pattern, before);
+  return emit_pattern_before (pattern, before, true, false,
+			      make_call_insn_raw);
 }
 
-/* like emit_insn_before_noloc, but set insn_locator according to scope.  */
+/* Like emit_insn_before_noloc, but set INSN_LOCATOR according to LOC.  */
 rtx
 emit_debug_insn_before_setloc (rtx pattern, rtx before, int loc)
 {
-  rtx first = PREV_INSN (before);
-  rtx last = emit_debug_insn_before_noloc (pattern, before);
-
-  if (pattern == NULL_RTX)
-    return last;
-
-  first = NEXT_INSN (first);
-  while (1)
-    {
-      if (active_insn_p (first) && !INSN_LOCATOR (first))
-	INSN_LOCATOR (first) = loc;
-      if (first == last)
-	break;
-      first = NEXT_INSN (first);
-    }
-  return last;
+  return emit_pattern_before_setloc (pattern, before, loc, false,
+				     make_debug_insn_raw);
 }
 
-/* like emit_debug_insn_before_noloc,
-   but set insn_locator according to before.  */
+/* Like emit_debug_insn_before_noloc,
+   but set insn_locator according to BEFORE.  */
 rtx
 emit_debug_insn_before (rtx pattern, rtx before)
 {
-  if (INSN_P (before))
-    return emit_debug_insn_before_setloc (pattern, before, INSN_LOCATOR (before));
-  else
-    return emit_debug_insn_before_noloc (pattern, before);
+  return emit_pattern_before (pattern, before, false, false,
+			      make_debug_insn_raw);
 }
 
 /* Take X and emit it at the end of the doubly-linked