Re: Fix minor reorg.c bug affecting MIPS targets

[Paul Hua <paul dot hua dot gm at gmail dot com>]

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;
> +}
>