| Message ID | ory4ayi25h.fsf@livre.home |
|---|---|
| State | New |
| Headers | show |
On Sat, Feb 06, 2016 at 08:06:18AM -0200, Alexandre Oliva wrote: > The testcase has a debug insn referencing a pseudo right before an > insn that modifies the pseudo. > > Without debug insns, REG_N_CALLS_CROSSED was zero for that pseudo, so > sched_analyze_reg added a dep between the pseudo setter and an earlier > (lib)call. > > With debug insns, we miscomputed REG_N_CALLS_CROSSED as nonzero > because of the debug insn, and then no dep was added between the two > insns. This was enough to change sched1's decisions about where to > place the pseudo setter. > > REG_N_CALLS_CROSSED is computed by both regstat_bb_compute_ri and > regstat_bb_compute_calls_crossed, but although the former skipped > debug insns, the latter didn't. > > Fixing this inconsistency was enough to fix the -fcompare-debug error. > > Regstrapped on x86_64-linux-gnu and i686-linux-gnu. Ok to install? Ok, with nit; thanks. > for gcc/ChangeLog > > PR target/69634 > * regstat.c (regstat_bb_compute_calls_crossed): Disregard > debug insns. > > for gcc/testsuite/ChangeLog > > PR target/69634 > * gcc.dg/pr69634.c: New. > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr69634.c > @@ -0,0 +1,22 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fno-dce -fschedule-insns -fno-tree-vrp -fcompare-debug" } */ > +/* { dg-additional-options "-Wno-psabi -mno-sse" { target i?86-*-* x86_64-*-* } } */ > +/* { dg-additional-options "-m32" { target x86_64-*-* } } */ Please remove the above line, -m32 should come solely from the user, and enough people either build i686-linux compilers or test with RUNTESTFLAGS='--target_board=unix\{-m64,-m32\}' that a regression in here would be noticed soon. Jakub
On 02/06/2016 03:06 AM, Alexandre Oliva wrote: > The testcase has a debug insn referencing a pseudo right before an > insn that modifies the pseudo. > > Without debug insns, REG_N_CALLS_CROSSED was zero for that pseudo, so > sched_analyze_reg added a dep between the pseudo setter and an earlier > (lib)call. > > With debug insns, we miscomputed REG_N_CALLS_CROSSED as nonzero > because of the debug insn, and then no dep was added between the two > insns. This was enough to change sched1's decisions about where to > place the pseudo setter. > > REG_N_CALLS_CROSSED is computed by both regstat_bb_compute_ri and > regstat_bb_compute_calls_crossed, but although the former skipped > debug insns, the latter didn't. > > Fixing this inconsistency was enough to fix the -fcompare-debug error. > > Regstrapped on x86_64-linux-gnu and i686-linux-gnu. Ok to install? > > > for gcc/ChangeLog > > PR target/69634 > * regstat.c (regstat_bb_compute_calls_crossed): Disregard > debug insns. > > for gcc/testsuite/ChangeLog > > PR target/69634 > * gcc.dg/pr69634.c: New. Code is fine. Testcase needs tweaking per Uros's comments in the BZ: Please don't use explicit -m32. If the test is valid only for 32bit target, you should use "target ia32", otherwise just leave it out. 32bit multilib tests will add -m32 automatically. Explicit -m32 will probably fail with x32, which is x86_64 target, too. With the test fixed this is fine. jeff
On 02/06/2016 03:06 AM, Alexandre Oliva wrote: > The testcase has a debug insn referencing a pseudo right before an > insn that modifies the pseudo. > > Without debug insns, REG_N_CALLS_CROSSED was zero for that pseudo, so > sched_analyze_reg added a dep between the pseudo setter and an earlier > (lib)call. > > With debug insns, we miscomputed REG_N_CALLS_CROSSED as nonzero > because of the debug insn, and then no dep was added between the two > insns. This was enough to change sched1's decisions about where to > place the pseudo setter. > > REG_N_CALLS_CROSSED is computed by both regstat_bb_compute_ri and > regstat_bb_compute_calls_crossed, but although the former skipped > debug insns, the latter didn't. > > Fixing this inconsistency was enough to fix the -fcompare-debug error. > > Regstrapped on x86_64-linux-gnu and i686-linux-gnu. Ok to install? > > > for gcc/ChangeLog > > PR target/69634 > * regstat.c (regstat_bb_compute_calls_crossed): Disregard > debug insns. > > for gcc/testsuite/ChangeLog > > PR target/69634 > * gcc.dg/pr69634.c: New. I removed the explicit -m32. It was there merely to try and exercise the test, even on an x86-64 configured toolchain. As Uros noted, a standard multilib of x86-64 will test -m32, so the explicit -m32 isn't needed and it is in fact harmful. Committed to the trunk with that change. I'm going to add 4.9/5 regression markers to the BZ since this affects those releases as well. Jeff
Alexandre Oliva <aoliva@redhat.com> writes: > diff --git a/gcc/testsuite/gcc.dg/pr69634.c b/gcc/testsuite/gcc.dg/pr69634.c > new file mode 100644 > index 0000000..837bd57 > --- /dev/null > +++ b/gcc/testsuite/gcc.dg/pr69634.c > @@ -0,0 +1,22 @@ > +/* { dg-do compile } */ > +/* { dg-options "-O2 -fno-dce -fschedule-insns -fno-tree-vrp -fcompare-debug" } */ > +/* { dg-additional-options "-Wno-psabi -mno-sse" { target i?86-*-* x86_64-*-* } } */ > +/* { dg-additional-options "-m32" { target x86_64-*-* } } */ > + > +typedef unsigned short u16; > +typedef short v16u16 __attribute__ ((vector_size (16))); > +typedef unsigned v16u32 __attribute__ ((vector_size (16))); > +typedef unsigned long long v16u64 __attribute__ ((vector_size (16))); > + > +u16 > +foo(u16 u16_1, v16u16 v16u16_0, v16u32 v16u64_0, v16u16 v16u16_1, v16u32 v16u32_1, v16u64 v16u64_1) On powerpc -m32: FAIL: gcc.dg/pr69634.c (test for excess errors) Excess errors: /daten/gcc/gcc-20160307/gcc/testsuite/gcc.dg/pr69634.c:11:1: warning: GCC vector passed by reference: non-standard ABI extension with no compatibility guarantee Andreas.
diff --git a/gcc/regstat.c b/gcc/regstat.c index af5e475..c05b69f 100644 --- a/gcc/regstat.c +++ b/gcc/regstat.c @@ -444,7 +444,7 @@ regstat_bb_compute_calls_crossed (unsigned int bb_index, bitmap live) struct df_insn_info *insn_info = DF_INSN_INFO_GET (insn); unsigned int regno; - if (!INSN_P (insn)) + if (!NONDEBUG_INSN_P (insn)) continue; /* Process the defs. */ diff --git a/gcc/testsuite/gcc.dg/pr69634.c b/gcc/testsuite/gcc.dg/pr69634.c new file mode 100644 index 0000000..837bd57 --- /dev/null +++ b/gcc/testsuite/gcc.dg/pr69634.c @@ -0,0 +1,22 @@ +/* { dg-do compile } */ +/* { dg-options "-O2 -fno-dce -fschedule-insns -fno-tree-vrp -fcompare-debug" } */ +/* { dg-additional-options "-Wno-psabi -mno-sse" { target i?86-*-* x86_64-*-* } } */ +/* { dg-additional-options "-m32" { target x86_64-*-* } } */ + +typedef unsigned short u16; +typedef short v16u16 __attribute__ ((vector_size (16))); +typedef unsigned v16u32 __attribute__ ((vector_size (16))); +typedef unsigned long long v16u64 __attribute__ ((vector_size (16))); + +u16 +foo(u16 u16_1, v16u16 v16u16_0, v16u32 v16u64_0, v16u16 v16u16_1, v16u32 v16u32_1, v16u64 v16u64_1) +{ + v16u64_1 /= (v16u64){~v16u32_1[1]}; + u16_1 = 0; + u16_1 /= v16u32_1[2]; + v16u64_1 -= (v16u64) v16u16_1; + u16_1 >>= 1; + u16_1 -= ~0; + v16u16_1 /= (v16u16){~u16_1, 1 - v16u64_0[0], 0xffb6}; + return u16_1 + v16u16_0[1] + v16u16_1[3] + v16u64_1[0] + v16u64_1[1]; +}