Re: [patch] PR56729
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
Re: [patch] PR56729
> 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.
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 ();
Re: [patch] PR56729
On Fri, Mar 29, 2013 at 1:05 PM, Steven Bosscher wrote: > Hello, > > 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. > > Bootstrapped&tested on powerpc64-unknown-linux-gnu (also tested 32bits ppc). > OK for trunk? Ok. Thanks, Richard. > Ciao! > Steven > > > PR middle-end/56729 > * df-scan.c (df_insn_delete): Disable failing assert. > > Index: df-scan.c > === > --- df-scan.c (revision 197180) > +++ df-scan.c (working copy) > @@ -1158,8 +1158,17 @@ 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 ();
[patch] PR56729
Hello, 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. Bootstrapped&tested on powerpc64-unknown-linux-gnu (also tested 32bits ppc). OK for trunk? Ciao! Steven PR middle-end/56729 * df-scan.c (df_insn_delete): Disable failing assert. Index: df-scan.c === --- df-scan.c (revision 197180) +++ df-scan.c (working copy) @@ -1158,8 +1158,17 @@ 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 ();
