[committed] Fix latent bug due to peephole2 pass dropping REG_INC notes

Message ID c6adb4dd43505ab44c1d656fc6f7b0168eea7a3f.camel@redhat.com
State New
Headers show
Series
  • [committed] Fix latent bug due to peephole2 pass dropping REG_INC notes
Related show

Commit Message

Kees Cook via Gcc-patches May 31, 2020, 5:20 p.m.
The H8 recently started regressing 20071219-1.c on the H8/SX with -mint32.  I
didn't really dig into what change caused the regression.  While I recently
changed this peephole, it was just collapsing 3 patterns into 1 using mode
iterators.  So more likely something earlier in the pipeline just changed things
enough to trigger this latent issue.



We have this peephole:

;; Convert a memory comparison to a move if there is a scratch register.

(define_peephole2
  [(match_scratch:QHSI 1 "r")
   (set (cc0)
        (compare (match_operand:QHSI 0 "memory_operand" "")
                 (const_int 0)))]
  ""
  [(set (match_dup 1)
        (match_dup 0))
   (set (cc0) (compare (match_dup 1)
                       (const_int 0)))]
  "")

So imagine if operand 0 has an embedded side effect.  We end up moving the
embedded side effect to a different insn but we end up losing our REG_INC note.

Later in reorg (yes, the H8/SX has delay slots, even though I think we rarely
fill them :(  we have this code:

>       /* If this insn is a register-register copy and the next insn has

>          a use of our destination, change it to use our source.  That way,

>          it will become a candidate for our delay slot the next time

>          through this loop.  This case occurs commonly in loops that

>          scan a list.

> 

>          We could check for more complex cases than those tested below,

>          but it doesn't seem worth it.  It might also be a good idea to try

>          to swap the two insns.  That might do better.

> 

>          We can't do this if the next insn modifies our destination, because

>          that would make the replacement into the insn invalid.  We also can't

>          do this if it modifies our source, because it might be an earlyclobber

>          operand.  This latter test also prevents updating the contents of

>          a PRE_INC.  We also can't do this if there's overlap of source and

>          destination.  Overlap may happen for larger-than-register-size

> modes.  */

> 

>       if (NONJUMP_INSN_P (trial) && GET_CODE (pat) == SET

>           && REG_P (SET_SRC (pat))

>           && REG_P (SET_DEST (pat))

>           && !reg_overlap_mentioned_p (SET_DEST (pat), SET_SRC (pat)))

>         {

>           rtx_insn *next = next_nonnote_insn (trial);

> 

>           if (next && NONJUMP_INSN_P (next)

>               && GET_CODE (PATTERN (next)) != USE

>               && ! reg_set_p (SET_DEST (pat), next)

>               && ! reg_set_p (SET_SRC (pat), next)

>               && reg_referenced_p (SET_DEST (pat), PATTERN (next))

>               && ! modified_in_p (SET_DEST (pat), next))

>             validate_replace_rtx (SET_DEST (pat), SET_SRC (pat), next);

>         }

>     }

> 


With the REG_INC note missing (it would have been detected in the reg_set_p
calls) this code fires and makes an invalid change resulting in incorrect code
being generated.

The peephole2 pass makes some attempt to update various notes, but that doesn't
include REG_INC notes.  While I could trivially fix this in the H8 port, I
wouldn't be terribly surprised if the lack of a REG_INC note could cause problems
on other ports.  So I think it's best to fix in the peephole pass.

As it turns out we already have  a function (two copies even!) to scan an insn
for auto-inc addressing modes and add an appropriate note.

This patch moves that code from reload & lra into rtlanal.c and calls it from the
peephole pass.

Bootstrapped and regression tested on x86-64, ppc64, ppc64le and a nice variety
of embedded targets have built and regression tested.  It's also bootstrapped on
m68k using qemu within a chroot (testing is ongoing).  Other targets are in
progress.  The tester has been a bit unstable due to various unrelated trunk
issues, so I don't have quite the coverage that I'd normally have at this point
(arm and aarch64 in particular would be useful to have, but have been highly
unstable lately).


Committing to the trunk,

Jeff
commit c25d0fa4d76cbc46078624d101ac019ff3df1142
Author: Jeff Law <law@redhat.com>
Date:   Sun May 31 11:16:37 2020 -0600

    Fix execute/20071219-1.c regression on H8 due to loss of REG_INC notes in peephole2.
    
    gcc/
            * lra.c (add_auto_inc_notes): Remove function.
            * reload1.c (add_auto_inc_notes): Similarly.  Move into...
            * rtlanal.c (add_auto_inc_notes): New function.
            * rtl.h (add_auto_inc_notes): Add prototype.
            * recog.c (peep2_attempt): Scan and add REG_INC notes to new insns
            as needed.

Comments

Oleg Endo May 31, 2020, 5:28 p.m. | #1
Hi Jeff,

On Sun, 2020-05-31 at 11:20 -0600, Jeff Law via Gcc-patches wrote:
> 

> The peephole2 pass makes some attempt to update various notes, but that doesn't

> include REG_INC notes.  While I could trivially fix this in the H8 port, I

> wouldn't be terribly surprised if the lack of a REG_INC note could cause problems

> on other ports.  So I think it's best to fix in the peephole pass.

> 

> As it turns out we already have  a function (two copies even!) to scan an insn

> for auto-inc addressing modes and add an appropriate note.

> 

> This patch moves that code from reload & lra into rtlanal.c and calls it from the

> peephole pass.


I ran into this issue a while ago.
See also config/sh/sh.c, function sh_check_add_incdec_notes.

Is it possible to somehow fold all that into a universal solution?

Cheers,
Oleg
Kees Cook via Gcc-patches May 31, 2020, 5:35 p.m. | #2
On Mon, 2020-06-01 at 02:28 +0900, Oleg Endo wrote:
> Hi Jeff,

> 

> On Sun, 2020-05-31 at 11:20 -0600, Jeff Law via Gcc-patches wrote:

> > The peephole2 pass makes some attempt to update various notes, but that

> > doesn't

> > include REG_INC notes.  While I could trivially fix this in the H8 port, I

> > wouldn't be terribly surprised if the lack of a REG_INC note could cause

> > problems

> > on other ports.  So I think it's best to fix in the peephole pass.

> > 

> > As it turns out we already have  a function (two copies even!) to scan an

> > insn

> > for auto-inc addressing modes and add an appropriate note.

> > 

> > This patch moves that code from reload & lra into rtlanal.c and calls it from

> > the

> > peephole pass.

> 

> I ran into this issue a while ago.

> See also config/sh/sh.c, function sh_check_add_incdec_notes.


> 

> Is it possible to somehow fold all that into a universal solution?

I think the only notable difference here is sh_check_add_incdec_notes verifies
there's no note on the INSN first.  I think if we added that check into the
generic code we could probably drop sh_check_add_incdec_notes.


Jeff

Patch

diff --git a/gcc/lra.c b/gcc/lra.c
index 5e8b75b1fda..3435cff6a1d 100644
--- a/gcc/lra.c
+++ b/gcc/lra.c
@@ -2231,34 +2231,6 @@  has_nonexceptional_receiver (void)
   return false;
 }
 
-
-/* Process recursively X of INSN and add REG_INC notes if necessary.  */
-static void
-add_auto_inc_notes (rtx_insn *insn, rtx x)
-{
-  enum rtx_code code = GET_CODE (x);
-  const char *fmt;
-  int i, j;
-
-  if (code == MEM && auto_inc_p (XEXP (x, 0)))
-    {
-      add_reg_note (insn, REG_INC, XEXP (XEXP (x, 0), 0));
-      return;
-    }
-
-  /* Scan all X sub-expressions.  */
-  fmt = GET_RTX_FORMAT (code);
-  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
-    {
-      if (fmt[i] == 'e')
-	add_auto_inc_notes (insn, XEXP (x, i));
-      else if (fmt[i] == 'E')
-	for (j = XVECLEN (x, i) - 1; j >= 0; j--)
-	  add_auto_inc_notes (insn, XVECEXP (x, i, j));
-    }
-}
-
-
 /* Remove all REG_DEAD and REG_UNUSED notes and regenerate REG_INC.
    We change pseudos by hard registers without notification of DF and
    that can make the notes obsolete.  DF-infrastructure does not deal
diff --git a/gcc/recog.c b/gcc/recog.c
index 8c098cf5b0f..25f19b1b1cf 100644
--- a/gcc/recog.c
+++ b/gcc/recog.c
@@ -3501,6 +3501,13 @@  peep2_attempt (basic_block bb, rtx_insn *insn, int match_len, rtx_insn *attempt)
   if (as_note)
     fixup_args_size_notes (before_try, last, get_args_size (as_note));
 
+  /* Scan the new insns for embedded side effects and add appropriate
+     REG_INC notes.  */
+  if (AUTO_INC_DEC)
+    for (x = last; x != before_try; x = PREV_INSN (x))
+      if (NONDEBUG_INSN_P (x))
+	add_auto_inc_notes (x, PATTERN (x));
+
   /* If we generated a jump instruction, it won't have
      JUMP_LABEL set.  Recompute after we're done.  */
   for (x = last; x != before_try; x = PREV_INSN (x))
diff --git a/gcc/reload1.c b/gcc/reload1.c
index 88f4727d545..19a64f2542a 100644
--- a/gcc/reload1.c
+++ b/gcc/reload1.c
@@ -395,7 +395,6 @@  static void delete_output_reload (rtx_insn *, int, int, rtx);
 static void delete_address_reloads (rtx_insn *, rtx_insn *);
 static void delete_address_reloads_1 (rtx_insn *, rtx, rtx_insn *);
 static void inc_for_reload (rtx, rtx, rtx, poly_int64);
-static void add_auto_inc_notes (rtx_insn *, rtx);
 static void substitute (rtx *, const_rtx, rtx);
 static bool gen_reload_chain_without_interm_reg_p (int, int);
 static int reloads_conflict (int, int);
@@ -9071,28 +9070,3 @@  inc_for_reload (rtx reloadreg, rtx in, rtx value, poly_int64 inc_amount)
 	emit_insn (gen_sub2_insn (reloadreg, inc));
     }
 }
-
-static void
-add_auto_inc_notes (rtx_insn *insn, rtx x)
-{
-  enum rtx_code code = GET_CODE (x);
-  const char *fmt;
-  int i, j;
-
-  if (code == MEM && auto_inc_p (XEXP (x, 0)))
-    {
-      add_reg_note (insn, REG_INC, XEXP (XEXP (x, 0), 0));
-      return;
-    }
-
-  /* Scan all the operand sub-expressions.  */
-  fmt = GET_RTX_FORMAT (code);
-  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
-    {
-      if (fmt[i] == 'e')
-	add_auto_inc_notes (insn, XEXP (x, i));
-      else if (fmt[i] == 'E')
-	for (j = XVECLEN (x, i) - 1; j >= 0; j--)
-	  add_auto_inc_notes (insn, XVECEXP (x, i, j));
-    }
-}
diff --git a/gcc/rtl.h b/gcc/rtl.h
index b0b1aacd2e8..0872cc408eb 100644
--- a/gcc/rtl.h
+++ b/gcc/rtl.h
@@ -3429,6 +3429,7 @@  extern rtx single_set_2 (const rtx_insn *, const_rtx);
 extern bool contains_symbol_ref_p (const_rtx);
 extern bool contains_symbolic_reference_p (const_rtx);
 extern bool contains_constant_pool_address_p (const_rtx);
+extern void add_auto_inc_notes (rtx_insn *, rtx);
 
 /* Handle the cheap and common cases inline for performance.  */
 
diff --git a/gcc/rtlanal.c b/gcc/rtlanal.c
index 9ff17caaba0..1d2874d8672 100644
--- a/gcc/rtlanal.c
+++ b/gcc/rtlanal.c
@@ -6591,3 +6591,29 @@  tls_referenced_p (const_rtx x)
       return true;
   return false;
 }
+
+/* Process recursively X of INSN and add REG_INC notes if necessary.  */
+void
+add_auto_inc_notes (rtx_insn *insn, rtx x)
+{
+  enum rtx_code code = GET_CODE (x);
+  const char *fmt;
+  int i, j;
+
+  if (code == MEM && auto_inc_p (XEXP (x, 0)))
+    {
+      add_reg_note (insn, REG_INC, XEXP (XEXP (x, 0), 0));
+      return;
+    }
+
+  /* Scan all X sub-expressions.  */
+  fmt = GET_RTX_FORMAT (code);
+  for (i = GET_RTX_LENGTH (code) - 1; i >= 0; i--)
+    {
+      if (fmt[i] == 'e')
+	add_auto_inc_notes (insn, XEXP (x, i));
+      else if (fmt[i] == 'E')
+	for (j = XVECLEN (x, i) - 1; j >= 0; j--)
+	  add_auto_inc_notes (insn, XVECEXP (x, i, j));
+    }
+}