[Bug target/20126] [3.3/3.4/4.0/4.1 Regression] Inlined memcmp makes one argument null on entry

["aoliva at redhat dot com" <gcc-bugzilla at gcc dot gnu dot org>]

------- Additional Comments From aoliva at gcc dot gnu dot org  2005-04-10 02:43 -------
Subject: Re: [PR target/20126, RFC] loop DEST_ADDR biv replacement may fail

On Apr  8, 2005, Roger Sayle <roger@eyesopen.com> wrote:

> ++     /* If there isn't a volatile MEM, there's nothing we can do.  */
> ++     || !for_each_rtx (&object, volatile_mem_p, 0)

This actually caused crashes.  We don't want to scan the entire insn
(it might contain NULLs), only the insn pattern.  Here's the patch
that bootstrapped and passed regression testing on amd64-linux-gnu.
Ok?

Index: gcc/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	PR target/20126
	* loop.c (loop_givs_rescan): If replacement of DEST_ADDR failed,
	set the original address pseudo to the correct value before the
	original insn, if possible, and leave the insn alone, otherwise
	create a new pseudo, set it and replace it in the insn.
	* recog.c (validate_change_maybe_volatile): New.
	* recog.h (validate_change_maybe_volatile): Declare.

Index: gcc/loop.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/loop.c,v
retrieving revision 1.525
diff -u -p -r1.525 loop.c
--- gcc/loop.c 2 Apr 2005 16:53:38 -0000 1.525
+++ gcc/loop.c 10 Apr 2005 00:13:56 -0000
@@ -5476,9 +5476,31 @@ loop_givs_rescan (struct loop *loop, str
 	mark_reg_pointer (v->new_reg, 0);
 
       if (v->giv_type == DEST_ADDR)
-	/* Store reduced reg as the address in the memref where we found
-	   this giv.  */
-	validate_change (v->insn, v->location, v->new_reg, 0);
+	{
+	  /* Store reduced reg as the address in the memref where we found
+	     this giv.  */
+	  if (validate_change_maybe_volatile (v->insn, v->location,
+					      v->new_reg))
+	    /* Yay, it worked!  */;
+	  /* Not replaceable; emit an insn to set the original
+	     giv reg from the reduced giv.  */
+	  else if (REG_P (*v->location))
+	    loop_insn_emit_before (loop, 0, v->insn,
+				   gen_move_insn (*v->location,
+						  v->new_reg));
+	  else
+	    {
+	      /* If it wasn't a reg, create a pseudo and use that.  */
+	      rtx reg, seq;
+	      start_sequence ();
+	      reg = force_reg (v->mode, *v->location);
+	      seq = get_insns ();
+	      end_sequence ();
+	      loop_insn_emit_before (loop, 0, v->insn, seq);
+	      if (!validate_change_maybe_volatile (v->insn, v->location, reg))
+		gcc_unreachable ();
+	    }
+	}
       else if (v->replaceable)
 	{
 	  reg_map[REGNO (v->dest_reg)] = v->new_reg;
Index: gcc/recog.c
===================================================================
RCS file: /cvs/gcc/gcc/gcc/recog.c,v
retrieving revision 1.221
diff -u -p -r1.221 recog.c
--- gcc/recog.c 7 Mar 2005 13:52:08 -0000 1.221
+++ gcc/recog.c 10 Apr 2005 00:13:57 -0000
@@ -235,6 +235,46 @@ validate_change (rtx object, rtx *loc, r
     return apply_change_group ();
 }
 
+
+/* Function to be passed to for_each_rtx to test whether a piece of
+   RTL contains any mem/v.  */
+static int
+volatile_mem_p (rtx *x, void *data ATTRIBUTE_UNUSED)
+{
+  return (MEM_P (*x) && MEM_VOLATILE_P (*x));
+}
+
+/* Same as validate_change, but doesn't support groups, and it accepts
+   volatile mems if they're already present in the original insn.  */
+
+int
+validate_change_maybe_volatile (rtx object, rtx *loc, rtx new)
+{
+  int result;
+
+  if (validate_change (object, loc, new, 0))
+    return 1;
+
+  if (volatile_ok
+      /* If there isn't a volatile MEM, there's nothing we can do.  */
+      || !for_each_rtx (&PATTERN (object), volatile_mem_p, 0)
+      /* Make sure we're not adding or removing volatile MEMs.  */
+      || for_each_rtx (loc, volatile_mem_p, 0)
+      || for_each_rtx (&new, volatile_mem_p, 0)
+      || !insn_invalid_p (object))
+    return 0;
+
+  volatile_ok = 1;
+
+  gcc_assert (!insn_invalid_p (object));
+
+  result = validate_change (object, loc, new, 0);
+
+  volatile_ok = 0;
+
+  return result;
+}
+
 /* This subroutine of apply_change_group verifies whether the changes to INSN
    were valid; i.e. whether INSN can still be recognized.  */
 
Index: gcc/recog.h
===================================================================
RCS file: /cvs/gcc/gcc/gcc/recog.h,v
retrieving revision 1.55
diff -u -p -r1.55 recog.h
--- gcc/recog.h 7 Mar 2005 13:52:09 -0000 1.55
+++ gcc/recog.h 10 Apr 2005 00:13:57 -0000
@@ -74,6 +74,7 @@ extern void init_recog_no_volatile (void
 extern int check_asm_operands (rtx);
 extern int asm_operand_ok (rtx, const char *);
 extern int validate_change (rtx, rtx *, rtx, int);
+extern int validate_change_maybe_volatile (rtx, rtx *, rtx);
 extern int insn_invalid_p (rtx);
 extern void confirm_change_group (void);
 extern int apply_change_group (void);
Index: gcc/testsuite/ChangeLog
from  Alexandre Oliva  <aoliva@redhat.com>

	* gcc.dg/pr20126.c: New.

Index: gcc/testsuite/gcc.dg/pr20126.c
===================================================================
RCS file: gcc/testsuite/gcc.dg/pr20126.c
diff -N gcc/testsuite/gcc.dg/pr20126.c
--- /dev/null	1 Jan 1970 00:00:00 -0000
+++ gcc/testsuite/gcc.dg/pr20126.c 10 Apr 2005 00:14:15 -0000
@@ -0,0 +1,50 @@
+/* dg-do run */
+/* dg-options "-O2" */
+
+/* PR target/20126 was not really target-specific, but rather a loop's
+   failure to take into account the possibility that a DEST_ADDR giv
+   replacement might fail, such as when you attempt to replace a REG
+   with a PLUS in one of the register_operands of cmpstrqi_rex_1.  */
+
+extern void abort (void);
+
+typedef struct { int a; char b[3]; } S;
+S c = { 2, "aa" }, d = { 2, "aa" };
+
+void *
+bar (const void *x, int y, int z)
+{
+  return (void *) 0;
+}
+
+int
+foo (S *x, S *y)
+{
+  const char *e, *f, *g;
+  int h;
+
+  h = y->a;
+  f = y->b;
+  e = x->b;
+
+  if (h == 1)
+    return bar (e, *f, x->a) != 0;
+
+  g = e + x->a - h;
+  while (e <= g)
+    {
+      const char *t = e + 1;
+      if (__builtin_memcmp (e, f, h) == 0)
+        return 1;
+      e = t;
+    }
+  return 0;
+}
+
+int
+main (void)
+{
+  if (foo (&c, &d) != 1)
+    abort ();
+  return 0;
+}

-- 
Alexandre Oliva             http://www.ic.unicamp.br/~oliva/
Red Hat Compiler Engineer   aoliva@{redhat.com, gcc.gnu.org}
Free Software Evangelist  oliva@{lsd.ic.unicamp.br, gnu.org}


-- 


http://gcc.gnu.org/bugzilla/show_bug.cgi?id=20126