diff mbox

[RFC,combine] PR48308 - Fix issue with missing(?) LOG_LINKS

Message ID CACUk7=X_8wyG3D2-Jqob6rZjXR1qN1EduVQJg+faLjnidhAYsg@mail.gmail.com
State Accepted
Headers show

Commit Message

Ramana Radhakrishnan Jan. 23, 2012, 3:58 p.m. UTC 
On 23 January 2012 09:02, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> Do you mean something like this instead ? I'm testing this - Ok if no
>> regressions ?
>
> OK with an appropriate ChangeLog.

Or not :( We really shouldn't use SUBST there as it only deals with
rtx *  It appeared better to invent a new SUBST for links given that
the type of this expression is struct insn_link rather than rtx * (Doh
!)  I did think about hacking up a local flag but somehow like this
version better.

It's survived a bootstrap and testrun with no regressions for
c,c++,fortran,ada on one of the power7 machines for
powerpc64-unknown-linux-gnu in the compile farm. I'll put it through
it's paces for trunk (arm-linux-gnueabi / x86_64) and 4.6 (modulo API
changes for alloc_insn_link) if you think this looks better.

cheers
Ramana



2012-01-23  Ramana Radhakrishnan  <ramana.radhakrishnan@linaro.org>

        PR rtl-optimization/48308
        * combine.c (do_SUBST_LINK): Declare and define.
        (SUBST_LINK): New.
        (try_combine): Handle LOG_LINKS for dummy i1 case.

>
> --
> Eric Botcazou

Comments

Eric Botcazou Jan. 23, 2012, 7:54 p.m. UTC | #1 
> Or not :( We really shouldn't use SUBST there as it only deals with
> rtx *  It appeared better to invent a new SUBST for links given that
> the type of this expression is struct insn_link rather than rtx * (Doh
> !)  I did think about hacking up a local flag but somehow like this
> version better.

OK, I forgot that LOG_LINKS weren't RTX.

> It's survived a bootstrap and testrun with no regressions for
> c,c++,fortran,ada on one of the power7 machines for
> powerpc64-unknown-linux-gnu in the compile farm. I'll put it through
> it's paces for trunk (arm-linux-gnueabi / x86_64) and 4.6 (modulo API
> changes for alloc_insn_link) if you think this looks better.

Sure, that's clearly better.

> 2012-01-23  Ramana Radhakrishnan  <ramana.radhakrishnan@linaro.org>
>
>         PR rtl-optimization/48308
>         * combine.c (do_SUBST_LINK): Declare and define.
>         (SUBST_LINK): New.
>         (try_combine): Handle LOG_LINKS for dummy i1 case.

You also need to document the changes made to enum undo_kind, struct undo, 
do_SUBST and undo_all.  OK with these changes.
diff mbox

Patch

Index: gcc/combine.c
===================================================================
--- gcc/combine.c	(revision 183416)
+++ gcc/combine.c	(working copy)
@@ -367,14 +367,14 @@ 
 /* Record one modification to rtl structure
    to be undone by storing old_contents into *where.  */
 
-enum undo_kind { UNDO_RTX, UNDO_INT, UNDO_MODE };
+enum undo_kind { UNDO_RTX, UNDO_INT, UNDO_MODE, UNDO_LINKS };
 
 struct undo
 {
   struct undo *next;
   enum undo_kind kind;
-  union { rtx r; int i; enum machine_mode m; } old_contents;
-  union { rtx *r; int *i; } where;
+  union { rtx r; int i; enum machine_mode m; struct insn_link *l; } old_contents;
+  union { rtx *r; int *i; struct insn_link **l; } where;
 };
 
 /* Record a bunch of changes to be undone, up to MAX_UNDO of them.
@@ -698,7 +698,8 @@ 
      that are perfectly valid, so we'd waste too much effort for
      little gain doing the checks here.  Focus on catching invalid
      transformations involving integer constants.  */
-  if (GET_MODE_CLASS (GET_MODE (oldval)) == MODE_INT
+  if (oldval
+      && GET_MODE_CLASS (GET_MODE (oldval)) == MODE_INT
       && CONST_INT_P (newval))
     {
       /* Sanity check that we're replacing oldval with a CONST_INT
@@ -789,6 +790,33 @@ 
 }
 
 #define SUBST_MODE(INTO, NEWVAL)  do_SUBST_MODE(&(INTO), (NEWVAL))
+
+/* Similar to SUBST, but NEWVAL is a LOG_LINKS expression.  */
+
+static void
+do_SUBST_LINK (struct insn_link **into, struct insn_link *newval)
+{
+  struct undo *buf;
+  struct insn_link * oldval = *into;
+
+  if (oldval == newval)
+    return;
+
+  if (undobuf.frees)
+    buf = undobuf.frees, undobuf.frees = buf->next;
+  else
+    buf = XNEW (struct undo);
+
+  buf->kind = UNDO_LINKS;
+  buf->where.l = into;
+  buf->old_contents.l = oldval;
+  *into = newval;
+
+  buf->next = undobuf.undos, undobuf.undos = buf;
+}
+
+#define SUBST_LINK(oldval, newval) do_SUBST_LINK (&oldval, newval)
+
 
 /* Subroutine of try_combine.  Determine whether the replacement patterns
    NEWPAT, NEWI2PAT and NEWOTHERPAT are cheaper according to insn_rtx_cost
@@ -2865,6 +2893,7 @@ 
 	  SUBST (PATTERN (i2), XVECEXP (PATTERN (i2), 0, 0));
 	  SUBST (XEXP (SET_SRC (PATTERN (i2)), 0),
 		 SET_DEST (PATTERN (i1)));
+	  SUBST_LINK (LOG_LINKS (i2), alloc_insn_link (i1, LOG_LINKS (i2)));
 	}
     }
 #endif
@@ -4494,6 +4523,9 @@ 
 	case UNDO_MODE:
 	  adjust_reg_mode (*undo->where.r, undo->old_contents.m);
 	  break;
+	case UNDO_LINKS:
+	  *undo->where.l = undo->old_contents.l;
+	  break;
 	default:
 	  gcc_unreachable ();
 	}