Patchwork PR56729

login
register
mail settings
Submitter Steven Bosscher
Date April 17, 2013, 8:12 p.m.
Message ID <CABu31nOYxjg5yhLSNdAzZ8X+wdicGRXa_2hsFE7GfO5Ud_YdBA@mail.gmail.com>
Download mbox | patch
Permalink /patch/237357/
State New
Headers show

Comments

Steven Bosscher - April 17, 2013, 8:12 p.m.
> On Fri, Mar 29, 2013 at 1:05 PM, Steven Bosscher wrote:
>> It looks like there are places in the middle end that use remove_insn
>> on insns that are not actually emitted. This breaks the assert I added
>> in df_insn_delete. The patch disables the assert for now. The comment
>> before the assert is now even messier than before but I think it's
>> better to explain why the assert cannot work than to remove the
>> comment and the assert altogether.

This is no longer necessary, now that remove_insn doesn't use
df_insn_delete. Only a small patch to lower-subreg.c is needed to
restore the check.

Bootstrapped&tested (unix{,-m32}) on powerpc64-unknown-linux-gnu.
OK for trunk?

Ciao!
Steven


        * lower-subreg.c (resolve_simple_move): If called self-recursive,
        do not delete_insn insns that have not yet been emitted, only
        unlink them with remove_insn.
        * df-scan.c (df_insn_delete): Revert r197492.
Jeff Law - April 17, 2013, 8:16 p.m.
On 04/17/2013 02:12 PM, Steven Bosscher wrote:
>> On Fri, Mar 29, 2013 at 1:05 PM, Steven Bosscher wrote:
>>> It looks like there are places in the middle end that use remove_insn
>>> on insns that are not actually emitted. This breaks the assert I added
>>> in df_insn_delete. The patch disables the assert for now. The comment
>>> before the assert is now even messier than before but I think it's
>>> better to explain why the assert cannot work than to remove the
>>> comment and the assert altogether.
>
> This is no longer necessary, now that remove_insn doesn't use
> df_insn_delete. Only a small patch to lower-subreg.c is needed to
> restore the check.
>
> Bootstrapped&tested (unix{,-m32}) on powerpc64-unknown-linux-gnu.
> OK for trunk?
>
> Ciao!
> Steven
>
>
>          * lower-subreg.c (resolve_simple_move): If called self-recursive,
>          do not delete_insn insns that have not yet been emitted, only
>          unlink them with remove_insn.
>          * df-scan.c (df_insn_delete): Revert r197492.
OK.
Jeff

Patch

Index: lower-subreg.c
===================================================================
--- lower-subreg.c      (revision 198002)
+++ lower-subreg.c      (working copy)
@@ -1069,7 +1069,13 @@  resolve_simple_move (rtx set, rtx insn)

   emit_insn_before (insns, insn);

-  delete_insn (insn);
+  /* If we get here via self-recursion, then INSN is not yet in the insns
+     chain and delete_insn will fail.  We only want to remove INSN from the
+     current sequence.  See PR56738.  */
+  if (in_sequence_p ())
+    remove_insn (insn);
+  else
+    delete_insn (insn);

   return insns;
 }
Index: df-scan.c
===================================================================
--- df-scan.c   (revision 198002)
+++ df-scan.c   (working copy)
@@ -1158,17 +1158,7 @@  df_insn_delete (rtx insn)
      In any case, we expect BB to be non-NULL at least up to register
      allocation, so disallow a non-NULL BB up to there.  Not perfect
      but better than nothing...  */
-  /* ??? bb can also be NULL if lower-subreg.c:resolve_simple_mov emits
-     an insn into a sequence and then does delete_insn on it.  Not sure
-     if that makes sense, but for now it means this assert cannot work.
-     See PR56738.
-     Disable for now but revisit before the end of GCC 4.9 stage1.  */
-#if 0
   gcc_checking_assert (bb != NULL || reload_completed);
-#else
-  if (bb == NULL)
-    return;
-#endif

   df_grow_bb_info (df_scan);
   df_grow_reg_info ();