Fix minor reorg.c bug affecting MIPS targets

Fix minor reorg.c bug affecting MIPS targets

[Jeff Law]

[-- Attachment #1: Type: text/plain, Size: 2829 bytes --]



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

[-- Attachment #2: P --]
[-- Type: text/plain, Size: 2832 bytes --]

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

Re: Fix minor reorg.c bug affecting MIPS targets

[Paul Hua]

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

RE: Fix minor reorg.c bug affecting MIPS targets

[Toma Tabacu]

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

Re: Fix minor reorg.c bug affecting MIPS targets

[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

RE: Fix minor reorg.c bug affecting MIPS targets

[Toma Tabacu]

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

Re: Fix minor reorg.c bug affecting MIPS targets

[Paul Hua]

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