| Message ID | 20101123172211.GT29412@tyan-ft48-01.lab.bos.redhat.com |
|---|---|
| State | New |
| Headers | show |
On 11/23/2010 12:22 PM, Jakub Jelinek wrote: > Hi! > > As mentioned in the PR, the testcase is miscompiled, because both > sched_analyze_insn and sched_analyze_2 assume that all JUMP_INSNs in > the deps->last_pending_memory_flush list were added there in > if (deps->pending_flush_length++> MAX_PENDING_LIST_LENGTH) > flush_pending_lists (deps, insn, true, true); > else > deps->last_pending_memory_flush > = alloc_INSN_LIST (insn, deps->last_pending_memory_flush);<==== here > and thus handles them as jumps. But, a JUMP_INSN may be added there > also through flush_pending_lists, be it in the above listed call > or in another spot, and at that point the JUMP_INSN doesn't stand > solely for itself, but also for all memory stores etc. on which > that insn depends. And such a JUMP_INSN should be handled like > all non-JUMP_INSN insns on the flush last_pending_memory_flush > list. > > Fixed by instead remembering if a JUMP_INSN stands only for itself > and nothing else in the deps->last_pending_memory_flush list > (such jumps have REG_NOTE_KIND set to REG_DEP_ANTI, could be > anything but REG_DEP_TRUE which is the default and used for > additions to the list from flush_pending_lists) and using REG_NOTE_KIND > instead of JUMP_P checks in the two places which care about it. > > Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > It looks ok for me, Jakub. Although it would be nice to add a comment about meaning REG_DEP_ANTI for element of list last_pending_memory_flush when you set it up in function deps_analyze_insn. Thanks. > 2010-11-23 Jakub Jelinek<jakub@redhat.com> > > PR rtl-optimization/46614 > * sched-deps.c (deps_analyze_insn): Mark JUMP_INSNs in > last_pending_memory_flush that weren't added through > flush_pending_lists with REG_DEP_ANTI. > (sched_analyze_2, sched_analyze_insn): Check REG_NOTE_KIND > on INSN_LIST instead of JUMP_P check on its operand. > * sched-rgn.c (concat_INSN_LIST): Copy over REG_NOTE_KIND. > > * gcc.dg/pr46614.c: New test. > > --- gcc/sched-deps.c.jj 2010-11-01 09:07:23.000000000 +0100 > +++ gcc/sched-deps.c 2010-11-23 15:59:44.000000000 +0100 > @@ -2484,7 +2484,7 @@ sched_analyze_2 (struct deps_desc *deps, > > for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1)) > { > - if (! JUMP_P (XEXP (u, 0))) > + if (REG_NOTE_KIND (u) != REG_DEP_ANTI) > add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI); > else if (deps_may_trap_p (x)) > { > @@ -2796,7 +2796,7 @@ sched_analyze_insn (struct deps_desc *de > REG_DEP_ANTI); > > for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1)) > - if (! JUMP_P (XEXP (u, 0)) > + if (REG_NOTE_KIND (u) != REG_DEP_ANTI > || !sel_sched_p ()) > add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI); > > @@ -3242,8 +3242,12 @@ deps_analyze_insn (struct deps_desc *dep > if (deps->pending_flush_length++> MAX_PENDING_LIST_LENGTH) > flush_pending_lists (deps, insn, true, true); > else > - deps->last_pending_memory_flush > - = alloc_INSN_LIST (insn, deps->last_pending_memory_flush); > + { > + deps->last_pending_memory_flush > + = alloc_INSN_LIST (insn, deps->last_pending_memory_flush); > + PUT_REG_NOTE_KIND (deps->last_pending_memory_flush, > + REG_DEP_ANTI); > + } > } > > sched_analyze_insn (deps, PATTERN (insn), insn); > --- gcc/sched-rgn.c.jj 2010-11-01 09:07:24.000000000 +0100 > +++ gcc/sched-rgn.c 2010-11-23 16:00:29.000000000 +0100 > @@ -2574,7 +2574,10 @@ concat_INSN_LIST (rtx copy, rtx old) > { > rtx new_rtx = old; > for (; copy ; copy = XEXP (copy, 1)) > - new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx); > + { > + new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx); > + PUT_REG_NOTE_KIND (new_rtx, REG_NOTE_KIND (copy)); > + } > return new_rtx; > } > > --- gcc/testsuite/gcc.dg/pr46614.c.jj 2010-11-23 16:06:07.000000000 +0100 > +++ gcc/testsuite/gcc.dg/pr46614.c 2010-11-23 16:05:44.000000000 +0100 > @@ -0,0 +1,56 @@ > +/* PR rtl-optimization/46614 */ > +/* { dg-do run } */ > +/* { dg-options "-O -fno-rename-registers -fsched2-use-superblocks -fschedule-insns2 -funroll-loops" } */ > + > +extern void abort (void); > + > +struct S > +{ > + unsigned char a; > + unsigned char b; > + unsigned int c; > + unsigned int e; > + unsigned char f; > + unsigned int g; > +}; > + > +void bar (struct S *x) > +{ > + int i; > + struct S *p = x; > + struct S r[16]; > + unsigned j; > + for (i = 0; i< 16; i++) > + { > + r[i].c = p->b + p->c; > + j = p->c + p->f; > + r[i].a = j + p->b; > + r[i].f = p->f + p->e; > + r[i].g = p->b + p->c; > + } > + for (i = 0; i< 16; i++) > + { > + if (r[i].c != x[i].b + x[i].c > + || r[i].a != x[i].c + x[i].f + x[i].b > + || r[i].f != x[i].f + x[i].e > + || r[i].g != x[i].b + x[i].c) > + abort (); > + } > + for (i = 0; i< 16; i++) > + { > + r[i].b = p->c; > + if (r[i].b != x[i].c) > + abort (); > + } > +} > + > +int > +main () > +{ > + int i; > + struct S x[16]; > + for (i = 0; i< 16; i++) > + x[i].b = x[i].c = x[i].e = x[i].f = 5; > + bar (x); > + return 0; > +} > > Jakub
On Tue, Nov 23, 2010 at 01:54:41PM -0500, Vladimir Makarov wrote: > On 11/23/2010 12:22 PM, Jakub Jelinek wrote: > >Fixed by instead remembering if a JUMP_INSN stands only for itself > >and nothing else in the deps->last_pending_memory_flush list > >(such jumps have REG_NOTE_KIND set to REG_DEP_ANTI, could be > >anything but REG_DEP_TRUE which is the default and used for > >additions to the list from flush_pending_lists) and using REG_NOTE_KIND > >instead of JUMP_P checks in the two places which care about it. > > > >Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? > > > It looks ok for me, Jakub. Although it would be nice to add a > comment about meaning REG_DEP_ANTI for element of list > last_pending_memory_flush when you set it up in function > deps_analyze_insn. Maybe also define macros? /* In deps->last_pending_memory_flush marks JUMP_INSNs that weren't added to the list because of flush_pending_lists, stands just for itself and not for any other pending memory reads/writes. */ #define NON_FLUSH_JUMP_KIND REG_DEP_ANTI #define NON_FLUSH_JUMP_P(x) (REG_NOTE_KIND (x) == REG_DEP_ANTI) Jakub
--- gcc/sched-deps.c.jj 2010-11-01 09:07:23.000000000 +0100 +++ gcc/sched-deps.c 2010-11-23 15:59:44.000000000 +0100 @@ -2484,7 +2484,7 @@ sched_analyze_2 (struct deps_desc *deps, for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1)) { - if (! JUMP_P (XEXP (u, 0))) + if (REG_NOTE_KIND (u) != REG_DEP_ANTI) add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI); else if (deps_may_trap_p (x)) { @@ -2796,7 +2796,7 @@ sched_analyze_insn (struct deps_desc *de REG_DEP_ANTI); for (u = deps->last_pending_memory_flush; u; u = XEXP (u, 1)) - if (! JUMP_P (XEXP (u, 0)) + if (REG_NOTE_KIND (u) != REG_DEP_ANTI || !sel_sched_p ()) add_dependence (insn, XEXP (u, 0), REG_DEP_ANTI); @@ -3242,8 +3242,12 @@ deps_analyze_insn (struct deps_desc *dep if (deps->pending_flush_length++ > MAX_PENDING_LIST_LENGTH) flush_pending_lists (deps, insn, true, true); else - deps->last_pending_memory_flush - = alloc_INSN_LIST (insn, deps->last_pending_memory_flush); + { + deps->last_pending_memory_flush + = alloc_INSN_LIST (insn, deps->last_pending_memory_flush); + PUT_REG_NOTE_KIND (deps->last_pending_memory_flush, + REG_DEP_ANTI); + } } sched_analyze_insn (deps, PATTERN (insn), insn); --- gcc/sched-rgn.c.jj 2010-11-01 09:07:24.000000000 +0100 +++ gcc/sched-rgn.c 2010-11-23 16:00:29.000000000 +0100 @@ -2574,7 +2574,10 @@ concat_INSN_LIST (rtx copy, rtx old) { rtx new_rtx = old; for (; copy ; copy = XEXP (copy, 1)) - new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx); + { + new_rtx = alloc_INSN_LIST (XEXP (copy, 0), new_rtx); + PUT_REG_NOTE_KIND (new_rtx, REG_NOTE_KIND (copy)); + } return new_rtx; } --- gcc/testsuite/gcc.dg/pr46614.c.jj 2010-11-23 16:06:07.000000000 +0100 +++ gcc/testsuite/gcc.dg/pr46614.c 2010-11-23 16:05:44.000000000 +0100 @@ -0,0 +1,56 @@ +/* PR rtl-optimization/46614 */ +/* { dg-do run } */ +/* { dg-options "-O -fno-rename-registers -fsched2-use-superblocks -fschedule-insns2 -funroll-loops" } */ + +extern void abort (void); + +struct S +{ + unsigned char a; + unsigned char b; + unsigned int c; + unsigned int e; + unsigned char f; + unsigned int g; +}; + +void bar (struct S *x) +{ + int i; + struct S *p = x; + struct S r[16]; + unsigned j; + for (i = 0; i < 16; i++) + { + r[i].c = p->b + p->c; + j = p->c + p->f; + r[i].a = j + p->b; + r[i].f = p->f + p->e; + r[i].g = p->b + p->c; + } + for (i = 0; i < 16; i++) + { + if (r[i].c != x[i].b + x[i].c + || r[i].a != x[i].c + x[i].f + x[i].b + || r[i].f != x[i].f + x[i].e + || r[i].g != x[i].b + x[i].c) + abort (); + } + for (i = 0; i < 16; i++) + { + r[i].b = p->c; + if (r[i].b != x[i].c) + abort (); + } +} + +int +main () +{ + int i; + struct S x[16]; + for (i = 0; i < 16; i++) + x[i].b = x[i].c = x[i].e = x[i].f = 5; + bar (x); + return 0; +}
Hi! As mentioned in the PR, the testcase is miscompiled, because both sched_analyze_insn and sched_analyze_2 assume that all JUMP_INSNs in the deps->last_pending_memory_flush list were added there in if (deps->pending_flush_length++ > MAX_PENDING_LIST_LENGTH) flush_pending_lists (deps, insn, true, true); else deps->last_pending_memory_flush = alloc_INSN_LIST (insn, deps->last_pending_memory_flush); <==== here and thus handles them as jumps. But, a JUMP_INSN may be added there also through flush_pending_lists, be it in the above listed call or in another spot, and at that point the JUMP_INSN doesn't stand solely for itself, but also for all memory stores etc. on which that insn depends. And such a JUMP_INSN should be handled like all non-JUMP_INSN insns on the flush last_pending_memory_flush list. Fixed by instead remembering if a JUMP_INSN stands only for itself and nothing else in the deps->last_pending_memory_flush list (such jumps have REG_NOTE_KIND set to REG_DEP_ANTI, could be anything but REG_DEP_TRUE which is the default and used for additions to the list from flush_pending_lists) and using REG_NOTE_KIND instead of JUMP_P checks in the two places which care about it. Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk? 2010-11-23 Jakub Jelinek <jakub@redhat.com> PR rtl-optimization/46614 * sched-deps.c (deps_analyze_insn): Mark JUMP_INSNs in last_pending_memory_flush that weren't added through flush_pending_lists with REG_DEP_ANTI. (sched_analyze_2, sched_analyze_insn): Check REG_NOTE_KIND on INSN_LIST instead of JUMP_P check on its operand. * sched-rgn.c (concat_INSN_LIST): Copy over REG_NOTE_KIND. * gcc.dg/pr46614.c: New test. Jakub