Fix reload ICE with define-split

Fix reload ICE with define-split

[Nathan Sidwell]

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

This patch fixes an ICE for m68k in reload caused by a define split.  Namely 
this assertion:

   /* A register that is incremented cannot be constant!  */
       gcc_assert (regno < FIRST_PSEUDO_REGISTER
               || reg_equiv_constant[regno] == 0);

This is failing on:

   (insn 110 35 111 1 (set (subreg:SI (reg:DI 50) 4)
             (mem/s:SI (post_inc:SI (reg/f:SI 37 [ ivtmp.39 ]))
         [5     per_cpu__kstat.irqs S4 A16])) 33 {*movsi_cf} (nil)
         (nil))

this instruction was originally:

(insn 37 35 38 1 (set (reg:DI 50)
         (zero_extend:DI (mem/s:SI (post_inc:SI (reg/f:SI 37 [ ivtmp.39 ])) [5 
per_cpu__kstat.irqs S4 A16]))) 63 {*zero_extendsidi2} (nil)
     (expr_list:REG_INC (reg/f:SI 37 [ ivtmp.39 ])
         (nil)))

but then a define_split (*zero_extendsidi2) processes it but does not propagate 
the REG_INC note.  The reg_inc note is lost.

So, local-alloc is only aware of an out-of-loop initialization of reg 37 that 
has a (correct) REG_EQUAL note attached to it.  local-alloc morphs the REG_EQUAL 
note into a REG_EQUIV note, giving:

(insn 33 32 90 0 (set (reg/f:SI 37 [ ivtmp.39 ])
         (const:SI (plus:SI (symbol_ref:SI ("per_cpu__kstat") [flags 0x40] 
<var_decl 0xf7f0a108 per_cpu__kstat>)
                 (const_int 40 [0x28])))) 33 {*movsi_cf} (nil)
     (expr_list:REG_EQUIV (const:SI (plus:SI (symbol_ref:SI ("per_cpu__kstat") 
[flags 0x40] <var_decl 0xf7f0a108 per_cpu__kstat>)
                 (const_int 40 [0x28])))


and then reload blows up when it discovers that reg 37 is not actually a constant.

After discussing this with Richard Sandiford, we agreed that the place to fix 
this was in the middle end, and not in m68k specific code.  To that end, this 
patch adds an update_reg_inc_notes_between function and calls it after splitting 
an instruction.

This fixes the attached testcase (part of the linux kernel).  I've also booted 
and tested i686-pc-linux-gnu with no regressions, ok?

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: all.diff --]
[-- Type: text/plain, Size: 5473 bytes --]

2007-02-23  Nathan Sidwell  <nathan@codesourcery.com>

	* flow.c (create_reg_inc_note, update_reg_inc_notes_between): New.
	* recog.c (split_insn): Call update_reg_inc_notes_between.
	* basic-block.h (update_reg_inc_notes_between): Declare.
	
	testsuite/
	* gcc.target/m68k/m68k.exp: New.
	* gcc.target/m68k/crash1.c: New.

Index: flow.c
===================================================================
--- flow.c	(revision 122227)
+++ flow.c	(working copy)
@@ -4652,6 +4652,63 @@ reg_set_to_hard_reg_set (HARD_REG_SET *t
       SET_HARD_REG_BIT (*to, i);
     }
 }
+
+#ifdef AUTO_INC_DEC
+/* If *XP is an auto increment/decrement, add a new reg note onto
+   *DATA, which is a list of reg notes.  */
+
+static int
+create_reg_inc_note (rtx *xp, void *data)
+{
+  rtx x = *xp;
+  rtx *notep = data;
+  
+  switch (GET_CODE (x))
+    {
+    case PRE_INC:
+    case PRE_DEC:
+    case POST_INC:
+    case POST_DEC:
+    case PRE_MODIFY:
+    case POST_MODIFY:
+      {
+	rtx reg = XEXP (x, 0);
+      
+	*notep = alloc_EXPR_LIST (REG_INC, reg, *notep);
+	return -1;
+      }
+    default:
+      break;
+    }
+  
+  return 0;
+}
+
+/* Update the REG_INC notes between INSN and END inclusive.  */
+
+void
+update_reg_inc_notes_between (rtx insn, rtx end)
+{
+  for (end = NEXT_INSN (end); insn != end; insn = NEXT_INSN (insn))
+    {
+      if (INSN_P (insn))
+	{
+	  rtx *notep = &REG_NOTES (insn);
+	  
+	  /* Remove any existing REG_INC notes */
+	  while (*notep)
+	    if (REG_NOTE_KIND (*notep) == REG_INC)
+	      *notep = XEXP (*notep, 1);
+	    else
+	      notep = &XEXP (*notep, 1);
+	  
+	  /* Create the new notes. */
+	  for_each_rtx (&PATTERN (insn), create_reg_inc_note,
+			&REG_NOTES (insn));
+	}
+    }
+}
+#endif /* AUTO_INC_DEC */
 \f
 
 static bool
Index: testsuite/gcc.target/m68k/m68k.exp
===================================================================
--- testsuite/gcc.target/m68k/m68k.exp	(revision 0)
+++ testsuite/gcc.target/m68k/m68k.exp	(revision 0)
@@ -0,0 +1,41 @@
+# Copyright (C) 1997, 2004, 2006 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Exit immediately if this isn't an m68k target.
+if ![istarget m68k*-*-*] then {
+  return
+}
+
+# Load support procs.
+load_lib gcc-dg.exp
+
+# If a testcase doesn't have special options, use these.
+global DEFAULT_CFLAGS
+if ![info exists DEFAULT_CFLAGS] then {
+    set DEFAULT_CFLAGS " -ansi -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cCS\]]] \
+	"" $DEFAULT_CFLAGS
+
+# All done.
+dg-finish
Index: testsuite/gcc.target/m68k/crash1.c
===================================================================
--- testsuite/gcc.target/m68k/crash1.c	(revision 0)
+++ testsuite/gcc.target/m68k/crash1.c	(revision 0)
@@ -0,0 +1,42 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fomit-frame-pointer" } */
+
+/* Caused an ICE because of forgotten auto increment.  */
+
+register void *current __asm__("%a2");
+
+struct kernel_stat
+{
+ long long user;
+ long long nice;
+ long long system;
+ long long idle;
+ long long steal;
+ unsigned irqs[256];
+};
+extern struct kernel_stat per_cpu__kstat;
+
+void show_stat(void)
+{
+  int i;
+  long long user, nice, system, idle, steal;
+  long long sum = 0;
+  
+  user = nice = system = idle = steal = 0;
+  for (i = 0; i < 1; i++)
+    {
+      int j;
+      user = user + per_cpu__kstat.user;
+      nice = nice + per_cpu__kstat.nice;
+      system = system + per_cpu__kstat.system;
+      idle = idle + per_cpu__kstat.idle;
+      steal = steal + per_cpu__kstat.steal;
+      
+      for (j = 0 ; j < 256 ; j++)
+	sum += per_cpu__kstat.irqs[j];
+    }
+  seq_printf(user, nice, system, idle, steal);
+  seq_printf(sum);
+  for (i = 0; i < 256; i++)
+    seq_printf (i);
+}
Index: recog.c
===================================================================
--- recog.c	(revision 122227)
+++ recog.c	(working copy)
@@ -2750,6 +2750,9 @@ split_insn (rtx insn)
 	  first = NEXT_INSN (first);
 	}
     }
+#ifdef AUTO_INC_DEC
+  update_reg_inc_notes_between (first, last);
+#endif
   return last;
 }
 
Index: basic-block.h
===================================================================
--- basic-block.h	(revision 122227)
+++ basic-block.h	(working copy)
@@ -851,6 +851,9 @@ extern int update_life_info (sbitmap, en
 extern int update_life_info_in_dirty_blocks (enum update_life_extent, int);
 extern int count_or_remove_death_notes (sbitmap, int);
 extern int propagate_block (basic_block, regset, regset, regset, int);
+#ifdef AUTO_INC_DEC
+extern void update_reg_inc_notes_between (rtx, rtx);
+#endif
 
 struct propagate_block_info;
 extern rtx propagate_one_insn (struct propagate_block_info *, rtx);

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Fri, 23 Feb 2007, Nathan Sidwell wrote:

> After discussing this with Richard Sandiford, we agreed that the place to fix
> this was in the middle end, and not in m68k specific code.  To that end, this
> patch adds an update_reg_inc_notes_between function and calls it after
> splitting an instruction.

I have a similiar patch, which I think is a bit better. try_split() 
already tries to recreate notes, so the patch below adds the hook for 
REG_INC notes there.

bye, Roman


	* emit-rtl.c (find_auto_inc): New.
	(try_split): recreate REG_INC notes.

Index: gcc-4.1/gcc/emit-rtl.c
===================================================================
--- gcc-4.1.orig/gcc/emit-rtl.c
+++ gcc-4.1/gcc/emit-rtl.c
@@ -3103,6 +3103,34 @@ prev_cc0_setter (rtx insn)
 }
 #endif
 
+#ifdef AUTO_INC_DEC
+static int
+find_auto_inc (rtx *xp, void *data)
+{
+  rtx x = *xp;
+  rtx reg = data;
+
+  if (GET_RTX_CLASS (GET_CODE (x)) != RTX_AUTOINC)
+    return 0;
+
+  switch (GET_CODE (x)) {
+  case PRE_DEC:
+  case PRE_INC:
+  case POST_DEC:
+  case POST_INC:
+  case PRE_MODIFY:
+  case POST_MODIFY:
+    if (rtx_equal_p (reg, XEXP (x, 0)))
+      return 1;
+    break;
+
+  default:
+    gcc_unreachable ();
+  }
+  return -1;
+}
+#endif
+
 /* Increment the label uses for all labels present in rtx.  */
 
 static void
@@ -3268,6 +3296,18 @@ try_split (rtx pat, rtx trial, int last)
 	    }
 	  break;
 
+#ifdef AUTO_INC_DEC
+	case REG_INC:
+	  for (insn = insn_last; insn; insn = PREV_INSN (insn))
+	    {
+	      rtx reg = XEXP (note, 0);
+	      if (for_each_rtx (&PATTERN (insn), find_auto_inc, reg) > 0)
+		REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, reg,
+						      REG_NOTES (insn));
+	    }
+	  break;
+#endif
+
 	default:
 	  break;
 	}

Re: Fix reload ICE with define-split

[Paolo Bonzini]

>> After discussing this with Richard Sandiford, we agreed that the place to fix
>> this was in the middle end, and not in m68k specific code.  To that end, this
>> patch adds an update_reg_inc_notes_between function and calls it after
>> splitting an instruction.
> 
> I have a similiar patch, which I think is a bit better. try_split() 
> already tries to recreate notes, so the patch below adds the hook for 
> REG_INC notes there.

This patch also has the benefit of working on dataflow branch (the 
problem with the other one is not insurmountable, it's just that flow.c 
is not present anymore on the branch).

Paolo

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Fri, 23 Feb 2007, Paolo Bonzini wrote:

> > I have a similiar patch, which I think is a bit better. try_split() already
> > tries to recreate notes, so the patch below adds the hook for REG_INC notes
> > there.
> 
> This patch also has the benefit of working on dataflow branch (the problem
> with the other one is not insurmountable, it's just that flow.c is not present
> anymore on the branch).

My point is that IMO it's better to update the REG_INC note together with 
the other notes and not separately. I don't know anything about the 
dataflow branch, so I can't say how it works there.

bye, Roman

Re: Fix reload ICE with define-split

[Nathan Sidwell]

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

Roman Zippel wrote:

> On Fri, 23 Feb 2007, Nathan Sidwell wrote:
> 
>> After discussing this with Richard Sandiford, we agreed that the place to fix
>> this was in the middle end, and not in m68k specific code.  To that end, this
>> patch adds an update_reg_inc_notes_between function and calls it after
>> splitting an instruction.
> 
> I have a similiar patch, which I think is a bit better. try_split() 
> already tries to recreate notes, so the patch below adds the hook for 
> REG_INC notes there.

aha, I guess that's safe, because the split should not be creating an auto 
increment out of nothing.

Here's an amalgamation of the two patches, which I've tested on a coldfire 
target and booted on i686-pc-linux-gnu, ok?

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk


[-- Attachment #2: all.diff --]
[-- Type: text/plain, Size: 5929 bytes --]

2007-02-23  Nathan Sidwell  <nathan@codesourcery.com>
	    Roman Zippel <zippel@linux-m68k.org>

	* emit-rtl.c (add_auto_inc_notes): New.
	(try_split): Use regular for loops rather than whiles.  Recreate
	REG_INC notes if there were any.

	testsuite/
	* gcc.target/m68k/m68k.exp: New.
	* gcc.target/m68k/crash1.c: New.

Index: emit-rtl.c
===================================================================
--- emit-rtl.c	(revision 122227)
+++ emit-rtl.c	(working copy)
@@ -3101,6 +3101,26 @@ mark_label_nuses (rtx x)
     }
 }
 
+#ifdef AUTO_INC_DEC
+/* Insert a REG_INC note to *DATA (a pointer to an RTX NOTE's slot) if
+   *XP is a RTX_AUTOINC class rtx.  */
+
+static int
+add_auto_inc_notes (rtx *xp, void *data)
+{
+  rtx x = *xp;
+  rtx *notep = data;
+
+  if (GET_RTX_CLASS (GET_CODE (x)) == RTX_AUTOINC)
+    {
+      rtx reg = XEXP (x, 0);
+      
+      *notep = alloc_EXPR_LIST (REG_INC, reg, *notep);
+    }
+  
+  return 0;
+}
+#endif
 \f
 /* Try splitting insns that can be split for better scheduling.
    PAT is the pattern which might split.
@@ -3201,47 +3221,43 @@ try_split (rtx pat, rtx trial, int last)
       switch (REG_NOTE_KIND (note))
 	{
 	case REG_EH_REGION:
-	  insn = insn_last;
-	  while (insn != NULL_RTX)
-	    {
-	      if (CALL_P (insn)
-		  || (flag_non_call_exceptions && INSN_P (insn)
-		      && may_trap_p (PATTERN (insn))))
-		REG_NOTES (insn)
-		  = gen_rtx_EXPR_LIST (REG_EH_REGION,
-				       XEXP (note, 0),
-				       REG_NOTES (insn));
-	      insn = PREV_INSN (insn);
-	    }
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
+	    if (CALL_P (insn)
+		|| (flag_non_call_exceptions && INSN_P (insn)
+		    && may_trap_p (PATTERN (insn))))
+	      REG_NOTES (insn)
+		= gen_rtx_EXPR_LIST (REG_EH_REGION,
+				     XEXP (note, 0),
+				     REG_NOTES (insn));
 	  break;
 
 	case REG_NORETURN:
 	case REG_SETJMP:
-	  insn = insn_last;
-	  while (insn != NULL_RTX)
-	    {
-	      if (CALL_P (insn))
-		REG_NOTES (insn)
-		  = gen_rtx_EXPR_LIST (REG_NOTE_KIND (note),
-				       XEXP (note, 0),
-				       REG_NOTES (insn));
-	      insn = PREV_INSN (insn);
-	    }
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
+	    if (CALL_P (insn))
+	      REG_NOTES (insn)
+		= gen_rtx_EXPR_LIST (REG_NOTE_KIND (note),
+				     XEXP (note, 0),
+				     REG_NOTES (insn));
 	  break;
 
 	case REG_NON_LOCAL_GOTO:
-	  insn = insn_last;
-	  while (insn != NULL_RTX)
-	    {
-	      if (JUMP_P (insn))
-		REG_NOTES (insn)
-		  = gen_rtx_EXPR_LIST (REG_NOTE_KIND (note),
-				       XEXP (note, 0),
-				       REG_NOTES (insn));
-	      insn = PREV_INSN (insn);
-	    }
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
+	    if (JUMP_P (insn))
+	      REG_NOTES (insn)
+		= gen_rtx_EXPR_LIST (REG_NOTE_KIND (note),
+				     XEXP (note, 0),
+				     REG_NOTES (insn));
 	  break;
-
+	  
+#ifdef AUTO_INC_DEC
+	case REG_INC:
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
+	    /* Create the new notes. */
+	    for_each_rtx (&PATTERN (insn), add_auto_inc_notes,
+			  &REG_NOTES (insn));
+	  break;
+#endif
 	default:
 	  break;
 	}
Index: testsuite/gcc.target/m68k/m68k.exp
===================================================================
--- testsuite/gcc.target/m68k/m68k.exp	(revision 0)
+++ testsuite/gcc.target/m68k/m68k.exp	(revision 0)
@@ -0,0 +1,41 @@
+# Copyright (C) 1997, 2004, 2006 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+# 
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+# 
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.  
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Exit immediately if this isn't an m68k target.
+if ![istarget m68k*-*-*] then {
+  return
+}
+
+# Load support procs.
+load_lib gcc-dg.exp
+
+# If a testcase doesn't have special options, use these.
+global DEFAULT_CFLAGS
+if ![info exists DEFAULT_CFLAGS] then {
+    set DEFAULT_CFLAGS " -ansi -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cCS\]]] \
+	"" $DEFAULT_CFLAGS
+
+# All done.
+dg-finish
Index: testsuite/gcc.target/m68k/crash1.c
===================================================================
--- testsuite/gcc.target/m68k/crash1.c	(revision 0)
+++ testsuite/gcc.target/m68k/crash1.c	(revision 0)
@@ -0,0 +1,42 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fomit-frame-pointer" } */
+
+/* Caused an ICE because of forgotten auto increment.  */
+
+register void *current __asm__("%a2");
+
+struct kernel_stat
+{
+ long long user;
+ long long nice;
+ long long system;
+ long long idle;
+ long long steal;
+ unsigned irqs[256];
+};
+extern struct kernel_stat per_cpu__kstat;
+
+void show_stat(void)
+{
+  int i;
+  long long user, nice, system, idle, steal;
+  long long sum = 0;
+  
+  user = nice = system = idle = steal = 0;
+  for (i = 0; i < 1; i++)
+    {
+      int j;
+      user = user + per_cpu__kstat.user;
+      nice = nice + per_cpu__kstat.nice;
+      system = system + per_cpu__kstat.system;
+      idle = idle + per_cpu__kstat.idle;
+      steal = steal + per_cpu__kstat.steal;
+      
+      for (j = 0 ; j < 256 ; j++)
+	sum += per_cpu__kstat.irqs[j];
+    }
+  seq_printf(user, nice, system, idle, steal);
+  seq_printf(sum);
+  for (i = 0; i < 256; i++)
+    seq_printf (i);
+}

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Mon, 26 Feb 2007, Nathan Sidwell wrote:

> +static int
> +add_auto_inc_notes (rtx *xp, void *data)
> +{
> +  rtx x = *xp;
> +  rtx *notep = data;
> +
> +  if (GET_RTX_CLASS (GET_CODE (x)) == RTX_AUTOINC)
> +    {
> +      rtx reg = XEXP (x, 0);
> +      
> +      *notep = alloc_EXPR_LIST (REG_INC, reg, *notep);
> +    }
> +  
> +  return 0;
> +}

This would add notes for all autoinc expressions (even for newly created 
by the split) and repeat it for every REG_INC note, but only if there was 
one already before.
I'm not sure how we should handle new autoinc expressions, some ports 
already add a note in this case (e.g. sh), so my patch specifically only 
recreated existing notes.
If we want to create all new REG_INC notes, I think it would be better to 
call this from try_split() unconditionally.

> aha, I guess that's safe, because the split should not be creating an auto
> increment out of nothing.

It seems, that sh is doing something like this.

bye, Roman

Re: Fix reload ICE with define-split

[Nathan Sidwell]

Roman Zippel wrote:
> Hi,
> 
> On Mon, 26 Feb 2007, Nathan Sidwell wrote:
> 
>> +static int
>> +add_auto_inc_notes (rtx *xp, void *data)
>> +{
>> +  rtx x = *xp;
>> +  rtx *notep = data;
>> +
>> +  if (GET_RTX_CLASS (GET_CODE (x)) == RTX_AUTOINC)
>> +    {
>> +      rtx reg = XEXP (x, 0);
>> +      
>> +      *notep = alloc_EXPR_LIST (REG_INC, reg, *notep);
>> +    }
>> +  
>> +  return 0;
>> +}
> 
> This would add notes for all autoinc expressions (even for newly created 
> by the split) and repeat it for every REG_INC note, but only if there was 
> one already before.

correct.

I can't tell if you're objecting or not.

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Mon, 26 Feb 2007, Nathan Sidwell wrote:

> > This would add notes for all autoinc expressions (even for newly created by
> > the split) and repeat it for every REG_INC note, but only if there was one
> > already before.
> 
> correct.
> 
> I can't tell if you're objecting or not.

An instruction can have multiple REG_INC notes and with your version it 
would add a complete set of REG_INC notes for every previous REG_INC note. 
IMO it's also a bit inconsequent to add REG_INC notes for a register, 
which didn't have one before, only if there was a REG_INC for a different 
register.
I'm fine with just recreating existing REG_INC notes or creating REG_INC 
for all autoinc expressions (even new ones), I'm just not sure about 
something inbetween.

bye, Roman

Re: Fix reload ICE with define-split

[Richard Sandiford]

Roman Zippel <zippel@linux-m68k.org> writes:
> On Fri, 23 Feb 2007, Nathan Sidwell wrote:
>> After discussing this with Richard Sandiford, we agreed that the
>> place to fix this was in the middle end, and not in m68k specific
>> code.  To that end, this patch adds an update_reg_inc_notes_between
>> function and calls it after splitting an instruction.
>
> I have a similiar patch, which I think is a bit better. try_split() 
> already tries to recreate notes, so the patch below adds the hook for 
> REG_INC notes there.

There's a behavioural difference here though.  Your patch copes only
with cases where the original insn had an automodified address,
whereas Nathan's copes with cases where the orignal insn didn't,
It's not unresonable to imagine a splitter that increments an
address and then decrements it again, leaving the final address the
same as the original, and AIUI it isn't the backend's responsibility
to add notes in that case.  So I think Nathan's is more correct in
principle.

Richard

Re: Fix reload ICE with define-split

[Nathan Sidwell]

Roman Zippel wrote:

> An instruction can have multiple REG_INC notes and with your version it 
> would add a complete set of REG_INC notes for every previous REG_INC note. 
> IMO it's also a bit inconsequent to add REG_INC notes for a register, 
> which didn't have one before, only if there was a REG_INC for a different 
> register.
> I'm fine with just recreating existing REG_INC notes or creating REG_INC 
> for all autoinc expressions (even new ones), I'm just not sure about 
> something inbetween.

I'm happy for an RTL maintainer to tell me what the right thing is :)

nathan
-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Mon, 26 Feb 2007, Richard Sandiford wrote:

> There's a behavioural difference here though.  Your patch copes only
> with cases where the original insn had an automodified address,
> whereas Nathan's copes with cases where the orignal insn didn't,
> It's not unresonable to imagine a splitter that increments an
> address and then decrements it again, leaving the final address the
> same as the original, and AIUI it isn't the backend's responsibility
> to add notes in that case.  So I think Nathan's is more correct in
> principle.

I don't disagree, see my other mail.
When I wrote the patch I looked through the other ports port and sh does 
something like you describe, but also adds the REG_INC notes. That was the
main reason why I only recreated existing notes.

bye, Roman

Re: Fix reload ICE with define-split

[Ian Lance Taylor]

Nathan Sidwell <nathan@codesourcery.com> writes:

> Roman Zippel wrote:
> 
> > An instruction can have multiple REG_INC notes and with your version
> > it would add a complete set of REG_INC notes for every previous
> > REG_INC note. IMO it's also a bit inconsequent to add REG_INC notes
> > for a register, which didn't have one before, only if there was a
> > REG_INC for a different register.
> > I'm fine with just recreating existing REG_INC notes or creating
> > REG_INC for all autoinc expressions (even new ones), I'm just not
> > sure about something inbetween.
> 
> I'm happy for an RTL maintainer to tell me what the right thing is :)

After define_split it needs to be the case that every autoincrement
has an associated REG_INC note.  Your second patch isn't right because
it adds all required REG_INC notes, but only if there was at least one
existing REG_INC note.

The question is: should we make the define_split responsible for
adding REG_INC notes when it introduces an autoincrement, or should we
slow down every define_split by searching for an RTX_AUTOINC code?  If
we choose the first option, then Roman's patch is correct.  If we
choose the second option, then your first patch is correct.

Existing backends already add REG_INC notes where required in a number
of different places.  So I am inclined to say that if a define_split
introduces an autoincrement, it should be responsible for adding a
REG_INC note (as is already done by sh.md).

So I'm going to approve Roman's patch, except that for increased
flexibility I think it should be modified to only create a REG_INC
note if the note does not already exist, since the backend may have
added the note itself.

Thanks.

Ian

Re: Fix reload ICE with define-split

[Nathan Sidwell]

Ian Lance Taylor wrote:

> So I'm going to approve Roman's patch, except that for increased
> flexibility I think it should be modified to only create a REG_INC
> note if the note does not already exist, since the backend may have
> added the note itself.

ok.  Roman, Please check in my testcase at least :)

nathan

-- 
Nathan Sidwell    ::   http://www.codesourcery.com   ::         CodeSourcery
nathan@codesourcery.com    ::     http://www.planetfall.pwp.blueyonder.co.uk

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Wed, 28 Feb 2007, Nathan Sidwell wrote:

> Ian Lance Taylor wrote:
> 
> > So I'm going to approve Roman's patch, except that for increased
> > flexibility I think it should be modified to only create a REG_INC
> > note if the note does not already exist, since the backend may have
> > added the note itself.
> 
> ok.  Roman, Please check in my testcase at least :)

Ok, I added the find_reg_note check and the test. I hope the change is 
simple enough so I don't have to retest the whole thing :) (which may take 
a few days), but I verified that it does the right thing with a cross 
compiler.

bye, Roman


2007-xx-xx  Roman Zippel <zippel@linux-m68k.org>
            Nathan Sidwell  <nathan@codesourcery.com>

        * emit-rtl.c (find_auto_inc): New.
        (try_split): recreate REG_INC notes.

        testsuite/
        * gcc.target/m68k/m68k.exp: New.
        * gcc.target/m68k/crash1.c: New.

---
 gcc/emit-rtl.c                         |   43 +++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/m68k/crash1.c |   42 ++++++++++++++++++++++++++++++++
 gcc/testsuite/gcc.target/m68k/m68k.exp |   41 +++++++++++++++++++++++++++++++
 3 files changed, 126 insertions(+)

Index: gcc/gcc/emit-rtl.c
===================================================================
--- gcc.orig/gcc/emit-rtl.c
+++ gcc/gcc/emit-rtl.c
@@ -3077,6 +3077,36 @@ prev_cc0_setter (rtx insn)
 }
 #endif
 
+#ifdef AUTO_INC_DEC
+/* Find a RTX_AUTOINC class rtx which matches DATA.  */
+
+static int
+find_auto_inc (rtx *xp, void *data)
+{
+  rtx x = *xp;
+  rtx reg = data;
+
+  if (GET_RTX_CLASS (GET_CODE (x)) != RTX_AUTOINC)
+    return 0;
+
+  switch (GET_CODE (x)) {
+  case PRE_DEC:
+  case PRE_INC:
+  case POST_DEC:
+  case POST_INC:
+  case PRE_MODIFY:
+  case POST_MODIFY:
+    if (rtx_equal_p (reg, XEXP (x, 0)))
+      return 1;
+    break;
+
+  default:
+    gcc_unreachable ();
+  }
+  return -1;
+}
+#endif
+
 /* Increment the label uses for all labels present in rtx.  */
 
 static void
@@ -3242,6 +3272,19 @@ try_split (rtx pat, rtx trial, int last)
 	    }
 	  break;
 
+#ifdef AUTO_INC_DEC
+	case REG_INC:
+	  for (insn = insn_last; insn; insn = PREV_INSN (insn))
+	    {
+	      rtx reg = XEXP (note, 0);
+	      if (!find_reg_note (insn, REG_INC, reg)
+		  && for_each_rtx (&PATTERN (insn), find_auto_inc, reg) > 0)
+		REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, reg,
+						      REG_NOTES (insn));
+	    }
+	  break;
+#endif
+
 	default:
 	  break;
 	}
Index: gcc/gcc/testsuite/gcc.target/m68k/crash1.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/m68k/crash1.c
@@ -0,0 +1,42 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fomit-frame-pointer" } */
+
+/* Caused an ICE because of forgotten auto increment.  */
+
+register void *current __asm__("%a2");
+
+struct kernel_stat
+{
+ long long user;
+ long long nice;
+ long long system;
+ long long idle;
+ long long steal;
+ unsigned irqs[256];
+};
+extern struct kernel_stat per_cpu__kstat;
+
+void show_stat(void)
+{
+  int i;
+  long long user, nice, system, idle, steal;
+  long long sum = 0;
+
+  user = nice = system = idle = steal = 0;
+  for (i = 0; i < 1; i++)
+    {
+      int j;
+      user = user + per_cpu__kstat.user;
+      nice = nice + per_cpu__kstat.nice;
+      system = system + per_cpu__kstat.system;
+      idle = idle + per_cpu__kstat.idle;
+      steal = steal + per_cpu__kstat.steal;
+
+      for (j = 0 ; j < 256 ; j++)
+	sum += per_cpu__kstat.irqs[j];
+    }
+  seq_printf(user, nice, system, idle, steal);
+  seq_printf(sum);
+  for (i = 0; i < 256; i++)
+    seq_printf (i);
+}
Index: gcc/gcc/testsuite/gcc.target/m68k/m68k.exp
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/m68k/m68k.exp
@@ -0,0 +1,41 @@
+# Copyright (C) 1997, 2004, 2006 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Exit immediately if this isn't an m68k target.
+if ![istarget m68k*-*-*] then {
+  return
+}
+
+# Load support procs.
+load_lib gcc-dg.exp
+
+# If a testcase doesn't have special options, use these.
+global DEFAULT_CFLAGS
+if ![info exists DEFAULT_CFLAGS] then {
+    set DEFAULT_CFLAGS " -ansi -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cCS\]]] \
+	"" $DEFAULT_CFLAGS
+
+# All done.
+dg-finish

Re: Fix reload ICE with define-split

[Ian Lance Taylor]

Roman Zippel <zippel@linux-m68k.org> writes:

> Ok, I added the find_reg_note check and the test. I hope the change is 
> simple enough so I don't have to retest the whole thing :) (which may take 
> a few days), but I verified that it does the right thing with a cross 
> compiler.

You do need to at least bootstrap this change on a primary platform.
You don't necessarily need to do so on an m68k.

> +  switch (GET_CODE (x)) {

Formatting is wrong--the { should be on the next line.

Thanks.

Ian

Re: Fix reload ICE with define-split

[Roman Zippel]

Hi,

On Thu, 28 Feb 2007, Ian Lance Taylor wrote:

> Roman Zippel <zippel@linux-m68k.org> writes:
> 
> > Ok, I added the find_reg_note check and the test. I hope the change is 
> > simple enough so I don't have to retest the whole thing :) (which may take 
> > a few days), but I verified that it does the right thing with a cross 
> > compiler.
> 
> You do need to at least bootstrap this change on a primary platform.
> You don't necessarily need to do so on an m68k.

Ok, I did this on i686-linux and also rechecked with a cross compiler.
I changed it to use FIND_REG_INC_NOTE and to make the bootstrap worthwile 
I also added the small look cleanups from Nathan's patch.

> > +  switch (GET_CODE (x)) {
> 
> Formatting is wrong--the { should be on the next line.

Damn habits. :)

bye, Roman


gcc/
2007-xx-xx  Roman Zippel <zippel@linux-m68k.org>
            Nathan Sidwell  <nathan@codesourcery.com>

        * emit-rtl.c (find_auto_inc): New.
        (try_split): recreate REG_INC notes,
	Use regular for loops rather than whiles.

gcc/testsuite/
2007-xx-xx  Nathan Sidwell  <nathan@codesourcery.com>
        * gcc.target/m68k/m68k.exp: New.
        * gcc.target/m68k/crash1.c: New.

---
 gcc/emit-rtl.c                         |   56 +++++++++++++++++++++++++++------
 gcc/testsuite/gcc.target/m68k/crash1.c |   42 ++++++++++++++++++++++++
 gcc/testsuite/gcc.target/m68k/m68k.exp |   41 ++++++++++++++++++++++++
 3 files changed, 130 insertions(+), 9 deletions(-)

Index: gcc/gcc/emit-rtl.c
===================================================================
--- gcc.orig/gcc/emit-rtl.c
+++ gcc/gcc/emit-rtl.c
@@ -3077,6 +3077,37 @@ prev_cc0_setter (rtx insn)
 }
 #endif
 
+#ifdef AUTO_INC_DEC
+/* Find a RTX_AUTOINC class rtx which matches DATA.  */
+
+static int
+find_auto_inc (rtx *xp, void *data)
+{
+  rtx x = *xp;
+  rtx reg = data;
+
+  if (GET_RTX_CLASS (GET_CODE (x)) != RTX_AUTOINC)
+    return 0;
+
+  switch (GET_CODE (x))
+    {
+      case PRE_DEC:
+      case PRE_INC:
+      case POST_DEC:
+      case POST_INC:
+      case PRE_MODIFY:
+      case POST_MODIFY:
+	if (rtx_equal_p (reg, XEXP (x, 0)))
+	  return 1;
+	break;
+
+      default:
+	gcc_unreachable ();
+    }
+  return -1;
+}
+#endif
+
 /* Increment the label uses for all labels present in rtx.  */
 
 static void
@@ -3201,8 +3232,7 @@ try_split (rtx pat, rtx trial, int last)
       switch (REG_NOTE_KIND (note))
 	{
 	case REG_EH_REGION:
-	  insn = insn_last;
-	  while (insn != NULL_RTX)
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
 	    {
 	      if (CALL_P (insn)
 		  || (flag_non_call_exceptions && INSN_P (insn)
@@ -3211,37 +3241,45 @@ try_split (rtx pat, rtx trial, int last)
 		  = gen_rtx_EXPR_LIST (REG_EH_REGION,
 				       XEXP (note, 0),
 				       REG_NOTES (insn));
-	      insn = PREV_INSN (insn);
 	    }
 	  break;
 
 	case REG_NORETURN:
 	case REG_SETJMP:
-	  insn = insn_last;
-	  while (insn != NULL_RTX)
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
 	    {
 	      if (CALL_P (insn))
 		REG_NOTES (insn)
 		  = gen_rtx_EXPR_LIST (REG_NOTE_KIND (note),
 				       XEXP (note, 0),
 				       REG_NOTES (insn));
-	      insn = PREV_INSN (insn);
 	    }
 	  break;
 
 	case REG_NON_LOCAL_GOTO:
-	  insn = insn_last;
-	  while (insn != NULL_RTX)
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
 	    {
 	      if (JUMP_P (insn))
 		REG_NOTES (insn)
 		  = gen_rtx_EXPR_LIST (REG_NOTE_KIND (note),
 				       XEXP (note, 0),
 				       REG_NOTES (insn));
-	      insn = PREV_INSN (insn);
 	    }
 	  break;
 
+#ifdef AUTO_INC_DEC
+	case REG_INC:
+	  for (insn = insn_last; insn != NULL_RTX; insn = PREV_INSN (insn))
+	    {
+	      rtx reg = XEXP (note, 0);
+	      if (!FIND_REG_INC_NOTE (insn, reg)
+		  && for_each_rtx (&PATTERN (insn), find_auto_inc, reg) > 0)
+		REG_NOTES (insn) = gen_rtx_EXPR_LIST (REG_INC, reg,
+						      REG_NOTES (insn));
+	    }
+	  break;
+#endif
+
 	default:
 	  break;
 	}
Index: gcc/gcc/testsuite/gcc.target/m68k/crash1.c
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/m68k/crash1.c
@@ -0,0 +1,42 @@
+/* { dg-do compile } */
+/* { dg-options "-Os -fomit-frame-pointer" } */
+
+/* Caused an ICE because of forgotten auto increment.  */
+
+register void *current __asm__("%a2");
+
+struct kernel_stat
+{
+ long long user;
+ long long nice;
+ long long system;
+ long long idle;
+ long long steal;
+ unsigned irqs[256];
+};
+extern struct kernel_stat per_cpu__kstat;
+
+void show_stat(void)
+{
+  int i;
+  long long user, nice, system, idle, steal;
+  long long sum = 0;
+
+  user = nice = system = idle = steal = 0;
+  for (i = 0; i < 1; i++)
+    {
+      int j;
+      user = user + per_cpu__kstat.user;
+      nice = nice + per_cpu__kstat.nice;
+      system = system + per_cpu__kstat.system;
+      idle = idle + per_cpu__kstat.idle;
+      steal = steal + per_cpu__kstat.steal;
+
+      for (j = 0 ; j < 256 ; j++)
+	sum += per_cpu__kstat.irqs[j];
+    }
+  seq_printf(user, nice, system, idle, steal);
+  seq_printf(sum);
+  for (i = 0; i < 256; i++)
+    seq_printf (i);
+}
Index: gcc/gcc/testsuite/gcc.target/m68k/m68k.exp
===================================================================
--- /dev/null
+++ gcc/gcc/testsuite/gcc.target/m68k/m68k.exp
@@ -0,0 +1,41 @@
+# Copyright (C) 1997, 2004, 2006 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 2 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with this program; if not, write to the Free Software
+# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA.
+
+# GCC testsuite that uses the `dg.exp' driver.
+
+# Exit immediately if this isn't an m68k target.
+if ![istarget m68k*-*-*] then {
+  return
+}
+
+# Load support procs.
+load_lib gcc-dg.exp
+
+# If a testcase doesn't have special options, use these.
+global DEFAULT_CFLAGS
+if ![info exists DEFAULT_CFLAGS] then {
+    set DEFAULT_CFLAGS " -ansi -pedantic-errors"
+}
+
+# Initialize `dg'.
+dg-init
+
+# Main loop.
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.\[cCS\]]] \
+	"" $DEFAULT_CFLAGS
+
+# All done.
+dg-finish

Re: Fix reload ICE with define-split

[Ian Lance Taylor]

Roman Zippel <zippel@linux-m68k.org> writes:

> gcc/
> 2007-xx-xx  Roman Zippel <zippel@linux-m68k.org>
>             Nathan Sidwell  <nathan@codesourcery.com>
> 
>         * emit-rtl.c (find_auto_inc): New.
>         (try_split): recreate REG_INC notes,
> 	Use regular for loops rather than whiles.
> 
> gcc/testsuite/
> 2007-xx-xx  Nathan Sidwell  <nathan@codesourcery.com>
>         * gcc.target/m68k/m68k.exp: New.
>         * gcc.target/m68k/crash1.c: New.

This is OK.

Thanks.

Ian