Message ID | 20101122220027.GR29412@tyan-ft48-01.lab.bos.redhat.com |
---|---|
State | New |
Headers | show |
On Mon, Nov 22, 2010 at 11:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > On the attached testcases jumpif* emits: > (jump_insn 8 7 9 (set (pc) > (label_ref 0)) pr46499-1.c:26 -1 > (nil)) > > (barrier 9 8 10) > > (jump_insn 10 9 11 (set (pc) > (label_ref 0)) pr46499-1.c:26 -1 > (nil)) > > (barrier 11 10 0) > where the label_ref in both cases is the non-fallthru label. This is > because ssa_name & ssa_name is the condition and both ssa_name's > are found using TER to be 0 (and the -fno-* options ensure it is not > optimized earlier). > maybe_cleanup_end_of_block is called to find out when the sequence ends > with an unconditional jump to the non-fallthru label, with no fallthru > code_label afterwards, which means all the sequence does (assuming no > side-effects in it but GIMPLE should ensure that) is that unconditional > jump and attempts to remove any other jumps from the sequence. > Apparently it was expecting just conditional jumps to the non-fallthru > label, for unconditional ones as in this example the following > barrier needs to be removed too, otherwise we end up with a barrier > in a middle of a bb and we either ICE on it, or miscompile it. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? Ok. Thanks, Richard. > 2010-11-22 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/46499 > * cfgexpand.c (maybe_cleanup_end_of_block): Remove also BARRIERs > following unconditional jumps. > > * gcc.dg/pr46499-1.c: New test. > * gcc.dg/pr46499-2.c: New test. > > --- gcc/cfgexpand.c.jj 2010-11-10 13:14:53.000000000 +0100 > +++ gcc/cfgexpand.c 2010-11-22 14:45:49.000000000 +0100 > @@ -1694,7 +1694,14 @@ maybe_cleanup_end_of_block (edge e, rtx > { > insn = PREV_INSN (insn); > if (JUMP_P (NEXT_INSN (insn))) > - delete_insn (NEXT_INSN (insn)); > + { > + if (!any_condjump_p (insn)) > + { > + gcc_assert (BARRIER_P (NEXT_INSN (NEXT_INSN (insn)))); > + delete_insn (NEXT_INSN (NEXT_INSN (insn))); > + } > + delete_insn (NEXT_INSN (insn)); > + } > } > } > } > --- gcc/testsuite/gcc.dg/pr46499-1.c.jj 2010-11-22 14:59:48.000000000 +0100 > +++ gcc/testsuite/gcc.dg/pr46499-1.c 2010-11-22 14:59:11.000000000 +0100 > @@ -0,0 +1,31 @@ > +/* PR middle-end/46499 */ > +/* { dg-do run } */ > +/* { dg-options "-O -fno-omit-frame-pointer -fno-tree-ccp -fno-tree-dominator-opts -finline-small-functions" } */ > + > +extern void abort (void); > + > +int count = 0; > + > +int > +foo (void) > +{ > + count++; > + return 0; > +} > + > +int > +bar (void) > +{ > + count++; > + return 0; > +} > + > +int > +main () > +{ > + if ((foo () == 1) & (bar () == 1)) > + abort (); > + if (count != 2) > + abort (); > + return 0; > +} > --- gcc/testsuite/gcc.dg/pr46499-2.c.jj 2010-11-22 14:59:53.000000000 +0100 > +++ gcc/testsuite/gcc.dg/pr46499-2.c 2010-11-22 14:55:37.000000000 +0100 > @@ -0,0 +1,19 @@ > +/* PR middle-end/46499 */ > +/* { dg-do compile } */ > +/* { dg-options "-O -fno-tree-ccp -fno-tree-dominator-opts" } */ > + > +extern void abort (void); > + > +static inline int > +foo (void) > +{ > + return 0; > +} > + > +int > +main () > +{ > + if ((foo () == 1) & (foo () == 1)) > + abort (); > + return 0; > +} > > Jakub >
On Mon, Nov 22, 2010 at 2:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > On the attached testcases jumpif* emits: > (jump_insn 8 7 9 (set (pc) > (label_ref 0)) pr46499-1.c:26 -1 > (nil)) > > (barrier 9 8 10) > > (jump_insn 10 9 11 (set (pc) > (label_ref 0)) pr46499-1.c:26 -1 > (nil)) > > (barrier 11 10 0) > where the label_ref in both cases is the non-fallthru label. This is > because ssa_name & ssa_name is the condition and both ssa_name's > are found using TER to be 0 (and the -fno-* options ensure it is not > optimized earlier). > maybe_cleanup_end_of_block is called to find out when the sequence ends > with an unconditional jump to the non-fallthru label, with no fallthru > code_label afterwards, which means all the sequence does (assuming no > side-effects in it but GIMPLE should ensure that) is that unconditional > jump and attempts to remove any other jumps from the sequence. > Apparently it was expecting just conditional jumps to the non-fallthru > label, for unconditional ones as in this example the following > barrier needs to be removed too, otherwise we end up with a barrier > in a middle of a bb and we either ICE on it, or miscompile it. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2010-11-22 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/46499 > * cfgexpand.c (maybe_cleanup_end_of_block): Remove also BARRIERs > following unconditional jumps. > This caused: http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46629
On Tue, Nov 23, 2010 at 1:59 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > On Mon, Nov 22, 2010 at 2:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: >> Hi! >> >> On the attached testcases jumpif* emits: >> (jump_insn 8 7 9 (set (pc) >> (label_ref 0)) pr46499-1.c:26 -1 >> (nil)) >> >> (barrier 9 8 10) >> >> (jump_insn 10 9 11 (set (pc) >> (label_ref 0)) pr46499-1.c:26 -1 >> (nil)) >> >> (barrier 11 10 0) >> where the label_ref in both cases is the non-fallthru label. This is >> because ssa_name & ssa_name is the condition and both ssa_name's >> are found using TER to be 0 (and the -fno-* options ensure it is not >> optimized earlier). >> maybe_cleanup_end_of_block is called to find out when the sequence ends >> with an unconditional jump to the non-fallthru label, with no fallthru >> code_label afterwards, which means all the sequence does (assuming no >> side-effects in it but GIMPLE should ensure that) is that unconditional >> jump and attempts to remove any other jumps from the sequence. >> Apparently it was expecting just conditional jumps to the non-fallthru >> label, for unconditional ones as in this example the following >> barrier needs to be removed too, otherwise we end up with a barrier >> in a middle of a bb and we either ICE on it, or miscompile it. >> >> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? >> >> 2010-11-22 Jakub Jelinek <jakub@redhat.com> >> >> PR middle-end/46499 >> * cfgexpand.c (maybe_cleanup_end_of_block): Remove also BARRIERs >> following unconditional jumps. >> > > This caused: > > http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46629 > LTO seems to generate unconditional jumps without barrier. Is this an LTO bug?
On Tue, Nov 23, 2010 at 2:01 PM, H.J. Lu <hjl.tools@gmail.com> wrote: > LTO seems to generate unconditional jumps without barrier. > Is this an LTO bug? Considering LTO does not work on the RTL level at all. I suspect you could get a C testcase that produces the same ICE. -- Pinski
On Mon, Nov 22, 2010 at 2:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: > Hi! > > On the attached testcases jumpif* emits: > (jump_insn 8 7 9 (set (pc) > (label_ref 0)) pr46499-1.c:26 -1 > (nil)) > > (barrier 9 8 10) > > (jump_insn 10 9 11 (set (pc) > (label_ref 0)) pr46499-1.c:26 -1 > (nil)) > > (barrier 11 10 0) > where the label_ref in both cases is the non-fallthru label. This is > because ssa_name & ssa_name is the condition and both ssa_name's > are found using TER to be 0 (and the -fno-* options ensure it is not > optimized earlier). > maybe_cleanup_end_of_block is called to find out when the sequence ends > with an unconditional jump to the non-fallthru label, with no fallthru > code_label afterwards, which means all the sequence does (assuming no > side-effects in it but GIMPLE should ensure that) is that unconditional > jump and attempts to remove any other jumps from the sequence. > Apparently it was expecting just conditional jumps to the non-fallthru > label, for unconditional ones as in this example the following > barrier needs to be removed too, otherwise we end up with a barrier > in a middle of a bb and we either ICE on it, or miscompile it. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > 2010-11-22 Jakub Jelinek <jakub@redhat.com> > > PR middle-end/46499 > * cfgexpand.c (maybe_cleanup_end_of_block): Remove also BARRIERs > following unconditional jumps. > > * gcc.dg/pr46499-1.c: New test. > * gcc.dg/pr46499-2.c: New test. > > --- gcc/cfgexpand.c.jj 2010-11-10 13:14:53.000000000 +0100 > +++ gcc/cfgexpand.c 2010-11-22 14:45:49.000000000 +0100 > @@ -1694,7 +1694,14 @@ maybe_cleanup_end_of_block (edge e, rtx > { > insn = PREV_INSN (insn); > if (JUMP_P (NEXT_INSN (insn))) > - delete_insn (NEXT_INSN (insn)); > + { > + if (!any_condjump_p (insn)) ^^^^^^ Shouldn't it be any_condjump_p (NEXT_INSN (insn)) > + { > + gcc_assert (BARRIER_P (NEXT_INSN (NEXT_INSN (insn)))); > + delete_insn (NEXT_INSN (NEXT_INSN (insn))); > + } > + delete_insn (NEXT_INSN (insn)); > + } > } > } > }
On Tue, Nov 23, 2010 at 04:51:35PM -0800, H.J. Lu wrote: > > --- gcc/cfgexpand.c.jj 2010-11-10 13:14:53.000000000 +0100 > > +++ gcc/cfgexpand.c 2010-11-22 14:45:49.000000000 +0100 > > @@ -1694,7 +1694,14 @@ maybe_cleanup_end_of_block (edge e, rtx > > { > > insn = PREV_INSN (insn); > > if (JUMP_P (NEXT_INSN (insn))) > > - delete_insn (NEXT_INSN (insn)); > > + { > > + if (!any_condjump_p (insn)) > ^^^^^^ Shouldn't it > be any_condjump_p (NEXT_INSN (insn)) Oops. No, but either if (!any_condjump_p (NEXT_INSN (insn))) or if (any_uncondjump_p (NEXT_INSN (insn))) I'll bootstrap/regtest the former now. Jakub
On Tue, Nov 23, 2010 at 5:00 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Nov 23, 2010 at 04:51:35PM -0800, H.J. Lu wrote: >> > --- gcc/cfgexpand.c.jj 2010-11-10 13:14:53.000000000 +0100 >> > +++ gcc/cfgexpand.c 2010-11-22 14:45:49.000000000 +0100 >> > @@ -1694,7 +1694,14 @@ maybe_cleanup_end_of_block (edge e, rtx >> > { >> > insn = PREV_INSN (insn); >> > if (JUMP_P (NEXT_INSN (insn))) >> > - delete_insn (NEXT_INSN (insn)); >> > + { >> > + if (!any_condjump_p (insn)) >> ^^^^^^ Shouldn't it >> be any_condjump_p (NEXT_INSN (insn)) > > Oops. > No, but either if (!any_condjump_p (NEXT_INSN (insn))) > or if (any_uncondjump_p (NEXT_INSN (insn))) > > I'll bootstrap/regtest the former now. Please add the testcase in http://gcc.gnu.org/bugzilla/show_bug.cgi?id=46629 Thanks.
--- gcc/cfgexpand.c.jj 2010-11-10 13:14:53.000000000 +0100 +++ gcc/cfgexpand.c 2010-11-22 14:45:49.000000000 +0100 @@ -1694,7 +1694,14 @@ maybe_cleanup_end_of_block (edge e, rtx { insn = PREV_INSN (insn); if (JUMP_P (NEXT_INSN (insn))) - delete_insn (NEXT_INSN (insn)); + { + if (!any_condjump_p (insn)) + { + gcc_assert (BARRIER_P (NEXT_INSN (NEXT_INSN (insn)))); + delete_insn (NEXT_INSN (NEXT_INSN (insn))); + } + delete_insn (NEXT_INSN (insn)); + } } } } --- gcc/testsuite/gcc.dg/pr46499-1.c.jj 2010-11-22 14:59:48.000000000 +0100 +++ gcc/testsuite/gcc.dg/pr46499-1.c 2010-11-22 14:59:11.000000000 +0100 @@ -0,0 +1,31 @@ +/* PR middle-end/46499 */ +/* { dg-do run } */ +/* { dg-options "-O -fno-omit-frame-pointer -fno-tree-ccp -fno-tree-dominator-opts -finline-small-functions" } */ + +extern void abort (void); + +int count = 0; + +int +foo (void) +{ + count++; + return 0; +} + +int +bar (void) +{ + count++; + return 0; +} + +int +main () +{ + if ((foo () == 1) & (bar () == 1)) + abort (); + if (count != 2) + abort (); + return 0; +} --- gcc/testsuite/gcc.dg/pr46499-2.c.jj 2010-11-22 14:59:53.000000000 +0100 +++ gcc/testsuite/gcc.dg/pr46499-2.c 2010-11-22 14:55:37.000000000 +0100 @@ -0,0 +1,19 @@ +/* PR middle-end/46499 */ +/* { dg-do compile } */ +/* { dg-options "-O -fno-tree-ccp -fno-tree-dominator-opts" } */ + +extern void abort (void); + +static inline int +foo (void) +{ + return 0; +} + +int +main () +{ + if ((foo () == 1) & (foo () == 1)) + abort (); + return 0; +}