| Message ID | CAKjxQH=BsgCVj8xKDeCnb1gBrxzM0ZFyLTF7a9NqTvtRG5WJ2w@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
Hello Paul, You're seeing this problem because mips.exp can't handle -O* in dg-options. The other tests in gcc.target/mips use a dg-skip-if to skip for -O0 and -O1 instead of having -O2 in dg-options. This is supposed to ensure that the tests are run for as many optimization levels as possible. I believe that Matthew can confirm this. Regards, Toma Tabacu > -----Original Message----- > From: gcc-patches-owner@gcc.gnu.org [mailto:gcc-patches-owner@gcc.gnu.org] On Behalf Of Paul Hua > Sent: 16 May 2017 10:11 > To: Jeff Law > Cc: gcc-patches > Subject: Re: Fix minor reorg.c bug affecting MIPS targets > > Hi: > > There are new failures between r248067 and r248036 on > mips64el-unknown-linux-gnu. > > ERROR: gcc.target/mips/reorgbug-1.c -O0 : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -O0 : Unrecognised > option: -O2 for " dg-options 1 "-O2 -msoft-float -mips2" " > ERROR: gcc.target/mips/reorgbug-1.c -O1 : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -O1 : Unrecognised > option: -O2 for " dg-options 1 "-O2 -msoft-float -mips2" " > ERROR: gcc.target/mips/reorgbug-1.c -O2 -flto > -fno-use-linker-plugin -flto-partition=none : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -O2 -flto > -fno-use-linker-plugin -flto-partition=none : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > ERROR: gcc.target/mips/reorgbug-1.c -O2 -flto -fuse-linker-plugin > -fno-fat-lto-objects : Unrecognised option: -O2 for " dg-options 1 > "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -O2 -flto > -fuse-linker-plugin -fno-fat-lto-objects : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > ERROR: gcc.target/mips/reorgbug-1.c -O2 : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -O2 : Unrecognised > option: -O2 for " dg-options 1 "-O2 -msoft-float -mips2" " > ERROR: gcc.target/mips/reorgbug-1.c -O3 -g : Unrecognised option: > -O2 for " dg-options 1 "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -O3 -g : Unrecognised > option: -O2 for " dg-options 1 "-O2 -msoft-float -mips2" " > ERROR: gcc.target/mips/reorgbug-1.c -Os : Unrecognised option: -O2 > for " dg-options 1 "-O2 -msoft-float -mips2" " > UNRESOLVED: gcc.target/mips/reorgbug-1.c -Os : Unrecognised > option: -O2 for " dg-options 1 "-O2 -msoft-float -mips2" " > > I don't know why? just delete the "-O2" options in dg-options, then > the test passed. > > diff --git a/gcc/testsuite/gcc.target/mips/reorgbug-1.c > b/gcc/testsuite/gcc.target/mips/reorgbug-1.c > index b820a2b..9537d21 100644 > --- a/gcc/testsuite/gcc.target/mips/reorgbug-1.c > +++ b/gcc/testsuite/gcc.target/mips/reorgbug-1.c > @@ -1,4 +1,4 @@ > -/* { dg-options "-O2 -msoft-float -mips2" } */ > +/* { dg-options "-msoft-float -mips2" } */ > > typedef long int __int32_t; > typedef long unsigned int __uint32_t; > > config info:Compiler version: 8.0.0 20170515 (experimental) (gcc trunk > r248067 mips64el o32 n32 n64) > Platform: mips64el-unknown-linux-gnu > configure flags: --prefix=/home/xuchenghua/toolchain/gcc-trunk-r248067 > --enable-bootstrap --enable-shared --enable-threads=posix > --enable-checking=release --enable-multilib--with-system-zlib > --enable-__cxa_atexit --disable-libunwind-exceptions > --enable-gnu-unique-object --enable-linker-build-id > --enable-languages=c,c++,fortran --enable-plugin > --enable-initfini-array --disable-libgcj --with-arch=mips64r2 > --with-abi=64 --with-multilib-list=32,n32,64 > --enable-gnu-indirect-function --with-long-double-128 > --build=mips64el-unknown-linux --target=mips64el-unknown-linux > --with-pkgversion='gcc trunk r248067 mips64el o32 n32 n64' > --disable-werror > > > paul > > On Tue, May 16, 2017 at 1:22 AM, Jeff Law <law@redhat.com> wrote: > > > > > > There's a subtle bug in reorg.c's relax_delay_slots that my tester tripped > > this weekend. Not sure what changed code generation wise as the affected > > port built just fine last week. But it is what is is. > > > > > > > > Assume before this code we've set TARGET_LABEL to the code_label associated > > with DELAY_JUMP_INSN (which is what we want)... > > > > > > > > /* If the first insn at TARGET_LABEL is redundant with a previous > > insn, redirect the jump to the following insn and process again. > > We use next_real_insn instead of next_active_insn so we > > don't skip USE-markers, or we'll end up with incorrect > > liveness info. */ > > > > [ ... ] > > > > /* Similarly, if it is an unconditional jump with one insn in its > > delay list and that insn is redundant, thread the jump. */ > > rtx_sequence *trial_seq = > > trial ? dyn_cast <rtx_sequence *> (PATTERN (trial)) : NULL; > > if (trial_seq > > && trial_seq->len () == 2 > > && JUMP_P (trial_seq->insn (0)) > > && simplejump_or_return_p (trial_seq->insn (0)) > > && redundant_insn (trial_seq->insn (1), insn, vNULL)) > > { > > target_label = JUMP_LABEL (trial_seq->insn (0)); > > if (ANY_RETURN_P (target_label)) > > target_label = find_end_label (target_label); > > > > if (target_label > > && redirect_with_delay_slots_safe_p (delay_jump_insn, > > target_label, insn)) > > { > > update_block (trial_seq->insn (1), insn); > > reorg_redirect_jump (delay_jump_insn, target_label); > > next = insn; > > continue; > > } > > } > > > > /* See if we have a simple (conditional) jump that is useless. */ > > if (! INSN_ANNULLED_BRANCH_P (delay_jump_insn) > > && ! condjump_in_parallel_p (delay_jump_insn) > > && prev_active_insn (as_a<rtx_insn *> (target_label)) == insn > > > > Now assume that we get into the TRUE arm of that first conditional which > > sets a new value for TARGET_LABEL. Normally when this happens in > > relax_delay_slots we're going to unconditionally continue. But as we can > > see there's an inner conditional and if we don't get into its true arm, then > > we'll pop out a nesting level and execute the second outer IF. > > > > That second outer IF assumes that TARGET_LABEL still points to the code > > label associated with DELAY_JUMP_INSN. Opps. In my particular case it was > > NULL and caused an ICE. But I could probably construct a case where it > > pointed to a real label and could result in incorrect code generation. > > > > The fix is pretty simple. Just creating a new variable (to avoid -Wshadow) > > inside that first outer IF is sufficient. > > > > Tested by building the affected target (mipsisa64r2el-elf) through newlib. > > > > Installed on the trunk. > > > > jeff > > > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog > > index 8cceb247a85..18b6ed59c73 100644 > > --- a/gcc/ChangeLog > > +++ b/gcc/ChangeLog > > @@ -1,5 +1,8 @@ > > 2017-05-15 Jeff Law <law@redhat.com> > > > > + * reorg.c (relax_delay_slots): Create a new variable to hold > > + the temporary target rather than clobbering TARGET_LABEL. > > + > > * config/tilegx/tilegx.c (tilegx_expand_unaligned_load): Add > > missing argument to extract_bit_field call. > > * config/tilepro/tilepro.c (tilepro_expand_unaligned_load): > > Likewise. > > diff --git a/gcc/reorg.c b/gcc/reorg.c > > index 85ef7d6880c..1a6fd86e286 100644 > > --- a/gcc/reorg.c > > +++ b/gcc/reorg.c > > @@ -3351,16 +3351,16 @@ relax_delay_slots (rtx_insn *first) > > && simplejump_or_return_p (trial_seq->insn (0)) > > && redundant_insn (trial_seq->insn (1), insn, vNULL)) > > { > > - target_label = JUMP_LABEL (trial_seq->insn (0)); > > - if (ANY_RETURN_P (target_label)) > > - target_label = find_end_label (target_label); > > + rtx temp_label = JUMP_LABEL (trial_seq->insn (0)); > > + if (ANY_RETURN_P (temp_label)) > > + temp_label = find_end_label (temp_label); > > > > - if (target_label > > + if (temp_label > > && redirect_with_delay_slots_safe_p (delay_jump_insn, > > - target_label, insn)) > > + temp_label, insn)) > > { > > update_block (trial_seq->insn (1), insn); > > - reorg_redirect_jump (delay_jump_insn, target_label); > > + reorg_redirect_jump (delay_jump_insn, temp_label); > > next = insn; > > continue; > > } > > diff --git a/gcc/testsuite/ChangeLog b/gcc/testsuite/ChangeLog > > index f198fc6b42b..9fb8c8598b8 100644 > > --- a/gcc/testsuite/ChangeLog > > +++ b/gcc/testsuite/ChangeLog > > @@ -1,3 +1,7 @@ > > +2017-05-15 Jeff Law <law@redhat.com> > > + > > + * gcc.target/mips/reorgbug-1.c: New test. > > + > > 2017-05-15 Pierre-Marie de Rodat <derodat@adacore.com> > > > > * gnat.dg/specs/pack13.ads: New test. > > diff --git a/gcc/testsuite/gcc.target/mips/reorgbug-1.c > > b/gcc/testsuite/gcc.target/mips/reorgbug-1.c > > new file mode 100644 > > index 00000000000..b820a2b5df1 > > --- /dev/null > > +++ b/gcc/testsuite/gcc.target/mips/reorgbug-1.c > > @@ -0,0 +1,39 @@ > > +/* { dg-options "-O2 -msoft-float -mips2" } */ > > + > > +typedef long int __int32_t; > > +typedef long unsigned int __uint32_t; > > +typedef union > > +{ > > + double value; > > + struct > > + { > > + __uint32_t msw; > > + __uint32_t lsw; > > + } > > + parts; > > +} > > +ieee_double_shape_type; > > +double > > +__ieee754_fmod (double x, double y, int z, int xx) > > +{ > > + __int32_t n, hx, hy, hz, ix, iy, sx, i; > > + __uint32_t lx, ly, lz; > > + ieee_double_shape_type ew_u; > > + ew_u.value = (x); > > + (lx) = ew_u.parts.lsw; > > + ew_u.value = (y); > > + (hy) = ew_u.parts.msw; > > + (ly) = ew_u.parts.lsw; > > + if (hy == 0 || hx >= 0x7ff00000) > > + return (x * y); > > + if (z) > > + { > > + if ((hx < hy) || (lx < ly)) > > + return x; > > + lz = lx - ly; > > + lx = lz + lz; > > + } > > + ieee_double_shape_type iw_u; > > + iw_u.parts.lsw = (lx); > > + return iw_u.value; > > +} > >
On 05/16/2017 10:01 AM, Toma Tabacu wrote: > Hello Paul, > > You're seeing this problem because mips.exp can't handle -O* in dg-options. > The other tests in gcc.target/mips use a dg-skip-if to skip for -O0 and -O1 instead of having -O2 in dg-options. > This is supposed to ensure that the tests are run for as many optimization levels as possible. > > I believe that Matthew can confirm this. Feel free to remove the -O2. The most important bits are -mips2 and -msoft-float. If nobody does it shortly, I'll take care of it myself. jeff
From: Jeff Law > On 05/16/2017 10:01 AM, Toma Tabacu wrote: >> Hello Paul, >> >> You're seeing this problem because mips.exp can't handle -O* in dg-options. >> The other tests in gcc.target/mips use a dg-skip-if to skip for -O0 and -O1 instead of having -O2 in dg-options. >> This is supposed to ensure that the tests are run for as many optimization levels as possible. >> >> I believe that Matthew can confirm this. > Feel free to remove the -O2. The most important bits are -mips2 and > -msoft-float. If nobody does it shortly, I'll take care of it myself. > >jeff Sorry, I won't be able to commit the fix until tomorrow. Thanks for taking care of this issue, though. Regards, Toma Tabacu
Hi: commited as r248137. Thanks, paul. On Wed, May 17, 2017 at 2:46 AM, Toma Tabacu <Toma.Tabacu@imgtec.com> wrote: > From: Jeff Law >> On 05/16/2017 10:01 AM, Toma Tabacu wrote: >>> Hello Paul, >>> >>> You're seeing this problem because mips.exp can't handle -O* in dg-options. >>> The other tests in gcc.target/mips use a dg-skip-if to skip for -O0 and -O1 instead of having -O2 in dg-options. >>> This is supposed to ensure that the tests are run for as many optimization levels as possible. >>> >>> I believe that Matthew can confirm this. >> Feel free to remove the -O2. The most important bits are -mips2 and >> -msoft-float. If nobody does it shortly, I'll take care of it myself. >> >>jeff > > Sorry, I won't be able to commit the fix until tomorrow. > Thanks for taking care of this issue, though. > > Regards, > Toma Tabacu
diff --git a/gcc/testsuite/gcc.target/mips/reorgbug-1.c b/gcc/testsuite/gcc.target/mips/reorgbug-1.c index b820a2b..9537d21 100644 --- a/gcc/testsuite/gcc.target/mips/reorgbug-1.c +++ b/gcc/testsuite/gcc.target/mips/reorgbug-1.c @@ -1,4 +1,4 @@ -/* { dg-options "-O2 -msoft-float -mips2" } */ +/* { dg-options "-msoft-float -mips2" } */ typedef long int __int32_t; typedef long unsigned int __uint32_t;