diff mbox

Fix endless recursion in decide_alg (PR target/69888)

Message ID 20160222211637.GQ3017@tucnak.redhat.com
State New
Headers show

Commit Message

Jakub Jelinek Feb. 22, 2016, 9:16 p.m. UTC 
Hi!

On the following testcase, we ICE due to infinite recursion in decide_alg
- max is 0, and expected_size is the same (2048) in the second and following
recursive calls.

Fixed by avoiding recursing with the same arguments as before.

Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?

2016-02-22  Jakub Jelinek  <jakub@redhat.com>

	PR target/69888
	* config/i386/i386.c (decide_alg): Ensure we don't recurse with
	identical arguments.  Formatting and spelling fixes.

	* gcc.target/i386/pr69888.c: New test.


	Jakub

Comments

Uros Bizjak Feb. 22, 2016, 9:27 p.m. UTC | #1 
On Mon, Feb 22, 2016 at 10:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
> Hi!
>
> On the following testcase, we ICE due to infinite recursion in decide_alg
> - max is 0, and expected_size is the same (2048) in the second and following
> recursive calls.
>
> Fixed by avoiding recursing with the same arguments as before.
>
> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>
> 2016-02-22  Jakub Jelinek  <jakub@redhat.com>
>
>         PR target/69888
>         * config/i386/i386.c (decide_alg): Ensure we don't recurse with
>         identical arguments.  Formatting and spelling fixes.
>
>         * gcc.target/i386/pr69888.c: New test.

OK with a nit below.

Thanks,
Uros.

>
> --- gcc/config/i386/i386.c.jj   2016-02-12 00:50:55.000000000 +0100
> +++ gcc/config/i386/i386.c      2016-02-22 10:25:53.564338145 +0100
> @@ -26032,11 +26032,12 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>             bool memset, bool zero_memset, bool have_as,
>             int *dynamic_check, bool *noalign)
>  {
> -  const struct stringop_algs * algs;
> +  const struct stringop_algs *algs;
>    bool optimize_for_speed;
>    int max = 0;
>    const struct processor_costs *cost;
>    int i;
> +  HOST_WIDE_INT orig_expected_size = expected_size;
>    bool any_alg_usable_p = false;
>
>    *noalign = false;
> @@ -26066,7 +26067,7 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>        any_alg_usable_p |= usable;
>
>        if (candidate != libcall && candidate && usable)
> -         max = algs->size[i].max;
> +       max = algs->size[i].max;
>      }
>
>    /* If expected size is not known but max size is small enough
> @@ -26076,7 +26077,7 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>        && expected_size == -1)
>      expected_size = min_size / 2 + max_size / 2;
>
> -  /* If user specified the algorithm, honnor it if possible.  */
> +  /* If user specified the algorithm, honor it if possible.  */
>    if (ix86_stringop_alg != no_stringop
>        && alg_usable_p (ix86_stringop_alg, memset, have_as))
>      return ix86_stringop_alg;
> @@ -26152,20 +26153,20 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>           || !alg_usable_p (algs->unknown_size, memset, have_as)))
>      {
>        enum stringop_alg alg;
> +      HOST_WIDE_INT new_expected_size = (max > 0 ? max : 4096) / 2;

      HOST_WIDE_INT new_expected_size = max > 0 ? max / 2 : 2048;

> -      /* If there aren't any usable algorithms, then recursing on
> -         smaller sizes isn't going to find anything.  Just return the
> -         simple byte-at-a-time copy loop.  */
> -      if (!any_alg_usable_p)
> +      /* If there aren't any usable algorithms or if recursing with the
> +        same arguments as before, then recursing on smaller sizes or
> +        same size isn't going to find anything.  Just return the simple
> +        byte-at-a-time copy loop.  */
> +      if (!any_alg_usable_p || orig_expected_size == new_expected_size)
>          {
>            /* Pick something reasonable.  */
>            if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
>              *dynamic_check = 128;
>            return loop_1_byte;
>          }
> -      if (max <= 0)
> -       max = 4096;
> -      alg = decide_alg (count, max / 2, min_size, max_size, memset,
> +      alg = decide_alg (count, new_expected_size, min_size, max_size, memset,
>                         zero_memset, have_as, dynamic_check, noalign);
>        gcc_assert (*dynamic_check == -1);
>        if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
> --- gcc/testsuite/gcc.target/i386/pr69888.c.jj  2016-02-22 10:31:52.529401396 +0100
> +++ gcc/testsuite/gcc.target/i386/pr69888.c     2016-02-22 10:34:10.852502628 +0100
> @@ -0,0 +1,10 @@
> +/* PR target/69888 */
> +/* { dg-do compile } */
> +/* { dg-options "-minline-all-stringops -mmemset-strategy=no_stringop:-1:noalign" } */
> +/* { dg-additional-options "-march=geode" { target ia32 } } */
> +
> +void
> +foo (char *p)
> +{
> +  __builtin_memset (p, 0, 32);
> +}
>
>         Jakub
Uros Bizjak Feb. 22, 2016, 9:32 p.m. UTC | #2 
On Mon, Feb 22, 2016 at 10:27 PM, Uros Bizjak <ubizjak@gmail.com> wrote:
> On Mon, Feb 22, 2016 at 10:16 PM, Jakub Jelinek <jakub@redhat.com> wrote:
>> Hi!
>>
>> On the following testcase, we ICE due to infinite recursion in decide_alg
>> - max is 0, and expected_size is the same (2048) in the second and following
>> recursive calls.
>>
>> Fixed by avoiding recursing with the same arguments as before.
>>
>> Bootstrapped/regtested on x86_64-linux and i686-linux, ok for trunk?
>>
>> 2016-02-22  Jakub Jelinek  <jakub@redhat.com>
>>
>>         PR target/69888
>>         * config/i386/i386.c (decide_alg): Ensure we don't recurse with
>>         identical arguments.  Formatting and spelling fixes.
>>
>>         * gcc.target/i386/pr69888.c: New test.
>
> OK with a nit below.

On the second thought, the patch is OK as is. It documents that new
expected size is half of the value.

Thanks,
Uros.

>>
>> --- gcc/config/i386/i386.c.jj   2016-02-12 00:50:55.000000000 +0100
>> +++ gcc/config/i386/i386.c      2016-02-22 10:25:53.564338145 +0100
>> @@ -26032,11 +26032,12 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>>             bool memset, bool zero_memset, bool have_as,
>>             int *dynamic_check, bool *noalign)
>>  {
>> -  const struct stringop_algs * algs;
>> +  const struct stringop_algs *algs;
>>    bool optimize_for_speed;
>>    int max = 0;
>>    const struct processor_costs *cost;
>>    int i;
>> +  HOST_WIDE_INT orig_expected_size = expected_size;
>>    bool any_alg_usable_p = false;
>>
>>    *noalign = false;
>> @@ -26066,7 +26067,7 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>>        any_alg_usable_p |= usable;
>>
>>        if (candidate != libcall && candidate && usable)
>> -         max = algs->size[i].max;
>> +       max = algs->size[i].max;
>>      }
>>
>>    /* If expected size is not known but max size is small enough
>> @@ -26076,7 +26077,7 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>>        && expected_size == -1)
>>      expected_size = min_size / 2 + max_size / 2;
>>
>> -  /* If user specified the algorithm, honnor it if possible.  */
>> +  /* If user specified the algorithm, honor it if possible.  */
>>    if (ix86_stringop_alg != no_stringop
>>        && alg_usable_p (ix86_stringop_alg, memset, have_as))
>>      return ix86_stringop_alg;
>> @@ -26152,20 +26153,20 @@ decide_alg (HOST_WIDE_INT count, HOST_WI
>>           || !alg_usable_p (algs->unknown_size, memset, have_as)))
>>      {
>>        enum stringop_alg alg;
>> +      HOST_WIDE_INT new_expected_size = (max > 0 ? max : 4096) / 2;
>
>       HOST_WIDE_INT new_expected_size = max > 0 ? max / 2 : 2048;
>
>> -      /* If there aren't any usable algorithms, then recursing on
>> -         smaller sizes isn't going to find anything.  Just return the
>> -         simple byte-at-a-time copy loop.  */
>> -      if (!any_alg_usable_p)
>> +      /* If there aren't any usable algorithms or if recursing with the
>> +        same arguments as before, then recursing on smaller sizes or
>> +        same size isn't going to find anything.  Just return the simple
>> +        byte-at-a-time copy loop.  */
>> +      if (!any_alg_usable_p || orig_expected_size == new_expected_size)
>>          {
>>            /* Pick something reasonable.  */
>>            if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
>>              *dynamic_check = 128;
>>            return loop_1_byte;
>>          }
>> -      if (max <= 0)
>> -       max = 4096;
>> -      alg = decide_alg (count, max / 2, min_size, max_size, memset,
>> +      alg = decide_alg (count, new_expected_size, min_size, max_size, memset,
>>                         zero_memset, have_as, dynamic_check, noalign);
>>        gcc_assert (*dynamic_check == -1);
>>        if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
>> --- gcc/testsuite/gcc.target/i386/pr69888.c.jj  2016-02-22 10:31:52.529401396 +0100
>> +++ gcc/testsuite/gcc.target/i386/pr69888.c     2016-02-22 10:34:10.852502628 +0100
>> @@ -0,0 +1,10 @@
>> +/* PR target/69888 */
>> +/* { dg-do compile } */
>> +/* { dg-options "-minline-all-stringops -mmemset-strategy=no_stringop:-1:noalign" } */
>> +/* { dg-additional-options "-march=geode" { target ia32 } } */
>> +
>> +void
>> +foo (char *p)
>> +{
>> +  __builtin_memset (p, 0, 32);
>> +}
>>
>>         Jakub
diff mbox

Patch

--- gcc/config/i386/i386.c.jj	2016-02-12 00:50:55.000000000 +0100
+++ gcc/config/i386/i386.c	2016-02-22 10:25:53.564338145 +0100
@@ -26032,11 +26032,12 @@  decide_alg (HOST_WIDE_INT count, HOST_WI
 	    bool memset, bool zero_memset, bool have_as,
 	    int *dynamic_check, bool *noalign)
 {
-  const struct stringop_algs * algs;
+  const struct stringop_algs *algs;
   bool optimize_for_speed;
   int max = 0;
   const struct processor_costs *cost;
   int i;
+  HOST_WIDE_INT orig_expected_size = expected_size;
   bool any_alg_usable_p = false;
 
   *noalign = false;
@@ -26066,7 +26067,7 @@  decide_alg (HOST_WIDE_INT count, HOST_WI
       any_alg_usable_p |= usable;
 
       if (candidate != libcall && candidate && usable)
-	  max = algs->size[i].max;
+	max = algs->size[i].max;
     }
 
   /* If expected size is not known but max size is small enough
@@ -26076,7 +26077,7 @@  decide_alg (HOST_WIDE_INT count, HOST_WI
       && expected_size == -1)
     expected_size = min_size / 2 + max_size / 2;
 
-  /* If user specified the algorithm, honnor it if possible.  */
+  /* If user specified the algorithm, honor it if possible.  */
   if (ix86_stringop_alg != no_stringop
       && alg_usable_p (ix86_stringop_alg, memset, have_as))
     return ix86_stringop_alg;
@@ -26152,20 +26153,20 @@  decide_alg (HOST_WIDE_INT count, HOST_WI
 	  || !alg_usable_p (algs->unknown_size, memset, have_as)))
     {
       enum stringop_alg alg;
+      HOST_WIDE_INT new_expected_size = (max > 0 ? max : 4096) / 2;
 
-      /* If there aren't any usable algorithms, then recursing on
-         smaller sizes isn't going to find anything.  Just return the
-         simple byte-at-a-time copy loop.  */
-      if (!any_alg_usable_p)
+      /* If there aren't any usable algorithms or if recursing with the
+	 same arguments as before, then recursing on smaller sizes or
+	 same size isn't going to find anything.  Just return the simple
+	 byte-at-a-time copy loop.  */
+      if (!any_alg_usable_p || orig_expected_size == new_expected_size)
         {
           /* Pick something reasonable.  */
           if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
             *dynamic_check = 128;
           return loop_1_byte;
         }
-      if (max <= 0)
-	max = 4096;
-      alg = decide_alg (count, max / 2, min_size, max_size, memset,
+      alg = decide_alg (count, new_expected_size, min_size, max_size, memset,
 			zero_memset, have_as, dynamic_check, noalign);
       gcc_assert (*dynamic_check == -1);
       if (TARGET_INLINE_STRINGOPS_DYNAMICALLY)
--- gcc/testsuite/gcc.target/i386/pr69888.c.jj	2016-02-22 10:31:52.529401396 +0100
+++ gcc/testsuite/gcc.target/i386/pr69888.c	2016-02-22 10:34:10.852502628 +0100
@@ -0,0 +1,10 @@ 
+/* PR target/69888 */
+/* { dg-do compile } */
+/* { dg-options "-minline-all-stringops -mmemset-strategy=no_stringop:-1:noalign" } */
+/* { dg-additional-options "-march=geode" { target ia32 } } */
+
+void
+foo (char *p)
+{
+  __builtin_memset (p, 0, 32);
+}