diff mbox

Fix twolf -funroll-loops -O3 miscompilation (a semi-latent web.c bug)

Message ID CABu31nM9+0Uf7BUkFxKGHmhdzK9jPx+zayDFU-=3+WRzhTyFCg@mail.gmail.com
State New
Headers show

Commit Message

Steven Bosscher Nov. 27, 2012, 11:29 p.m. UTC 
On Tue, Nov 27, 2012 at 5:57 PM, Eric Botcazou <ebotcazou@adacore.com> wrote:
>> This note seems very very weird. For one thing, it becomes invalid on
>> the very instruction where it is created.  I would say that it should
>> not be there.
>
> Agreed.

Count me in, too.  So let's avoid it:

        * gcse.c (struct reg_use): Remove unused struct.
        (gcse_emit_move_after): Do not create REG_EQUAL notes that reference
        the SET_DEST of the instruction the note would be attached to.

Comments

Eric Botcazou Nov. 27, 2012, 11:47 p.m. UTC | #1 
> Count me in, too.  So let's avoid it:
> 
>         * gcse.c (struct reg_use): Remove unused struct.
>         (gcse_emit_move_after): Do not create REG_EQUAL notes that reference
> the SET_DEST of the instruction the note would be attached to.

OK (with PR rtl-optimization/55006) if it passes bootstrap & regtest, thanks.

> And perhaps a bit in emit-rtl.c for good measure? I'll see if/where
> this causes breakage...

I think this would need to be wrapped in a #ifdef ENABLE_CHECKING/#endif pair.
diff mbox

Patch

Index: gcse.c
===================================================================
--- gcse.c      (revision 193394)
+++ gcse.c      (working copy)
@@ -258,8 +258,6 @@  int flag_rerun_cse_after_global_opts;
 /* An obstack for our working variables.  */
 static struct obstack gcse_obstack;

-struct reg_use {rtx reg_rtx; };
-
 /* Hash table of expressions.  */

 struct expr
@@ -2482,23 +2480,27 @@  gcse_emit_move_after (rtx dest, rtx src,
   rtx new_rtx;
   rtx set = single_set (insn), set2;
   rtx note;
-  rtx eqv;
+  rtx eqv = NULL_RTX;

   /* This should never fail since we're creating a reg->reg copy
      we've verified to be valid.  */

   new_rtx = emit_insn_after (gen_move_insn (dest, src), insn);

-  /* Note the equivalence for local CSE pass.  */
+  /* Note the equivalence for local CSE pass.  Take the note from the old
+     set if there was one.  Otherwise record the SET_SRC from the old set
+     unless DEST is also an operand of the SET_SRC.  */
   set2 = single_set (new_rtx);
   if (!set2 || !rtx_equal_p (SET_DEST (set2), dest))
     return new_rtx;
   if ((note = find_reg_equal_equiv_note (insn)))
     eqv = XEXP (note, 0);
-  else
+  else if (! REG_P (dest)
+          || ! reg_mentioned_p (dest, SET_SRC (set)))
     eqv = SET_SRC (set);

-  set_unique_reg_note (new_rtx, REG_EQUAL, copy_insn_1 (eqv));
+  if (eqv != NULL_RTX)
+    set_unique_reg_note (new_rtx, REG_EQUAL, copy_insn_1 (eqv));

   return new_rtx;
 }


And perhaps a bit in emit-rtl.c for good measure? I'll see if/where
this causes breakage...

Index: emit-rtl.c
===================================================================
--- emit-rtl.c  (revision 193394)
+++ emit-rtl.c  (working copy)
@@ -4932,6 +4932,19 @@  gen_use (rtx x)
   return seq;
 }

+/* Return true if DATUM has a USE of the SET_DEST of INSN.  */
+static bool
+self_ref_note_p (rtx insn, rtx datum)
+{
+  rtx set = single_set (insn);
+  if (! set)
+    return false;
+  rtx dest = SET_DEST (set);
+  if (! REG_P (dest))
+    return false;
+  return reg_mentioned_p (dest, datum);
+}
+
 /* Place a note of KIND on insn INSN with DATUM as the datum. If a
    note of this type already exists, remove it first.  */

@@ -4961,6 +4974,8 @@  set_unique_reg_note (rtx insn, enum reg_

       if (note)
        {
+         if (self_ref_note_p (insn, datum))
+           internal_error ("self-reference in REG_EQUAL or REG_EQUIV note!\n");
          XEXP (note, 0) = datum;
          df_notes_rescan (insn);
          return note;
@@ -4982,6 +4997,8 @@  set_unique_reg_note (rtx insn, enum reg_
     {
     case REG_EQUAL:
     case REG_EQUIV:
+      if (self_ref_note_p (insn, datum))
+       internal_error ("self-reference in REG_EQUAL or REG_EQUIV note!\n");
       df_notes_rescan (insn);
       break;
     default: